Skip to content

upgrade LSP7 and add Nethermind Audit report #16

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
JordyDutch merged 8 commits into from
Apr 19, 2025
Merged

upgrade LSP7 and add Nethermind Audit report #16

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d271d4e
fix: upgrade `@lukso/lsp7-contracts` to audited patch version
JordyDutch Apr 19, 2025
e5922e1
Add Nethermind Audit Agent report.
JordyDutch Apr 19, 2025
0bd23d9
fix: change dependency versions
JordyDutch Apr 19, 2025
e2ec92f
clean up
JordyDutch Apr 19, 2025
421a183
docs: improve grammar in Natspec comments
JordyDutch Apr 19, 2025
5f791a5
Add the address of sLYX token implementation
JordyDutch Apr 19, 2025
fce34b7
chore: add script to set token metadata
JordyDutch Apr 19, 2025
84ee678
chore: add script to set LSP4 Creators
JordyDutch Apr 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion .env.example
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,4 +54,10 @@ SLYX_TOKEN_CONTRACT_OWNER_ADDRESS="0x..."
NEW_SLYX_PROXY_ADMIN_ADDRESS="0x..."

# Parameter used when upgrading the SLYX Token proxy to a new implementation
NEW_SLYX_TOKEN_IMPLEMENTATION_ADDRESS="0x..."
NEW_SLYX_TOKEN_IMPLEMENTATION_ADDRESS="0x..."

# =======================
# SLYX Metadata parameters
# =======================

SLYX_TOKEN_METADATA_VALUE="0x..."
3 changes: 2 additions & 1 deletion .solhintignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
src/IDepositContract.sol
src/IDepositContract.sol
src/Vault.sol
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ Repository for the Stakingverse contracts. This repository includes the followin
| Staking Vault `Vault.sol` Implementation <br/> (commit [`33d1619` on Universal.Page repository](https://github.com/Universal-Page/contracts/tree/33d1619a19162444c870b8a5a4bf42eb4532818c)) | [`0x2Cb02ef26aDDAB15686ed634d70699ab64F195f4`](https://explorer.lukso.network/address/0x2Cb02ef26aDDAB15686ed634d70699ab64F195f4?tab=contract) |
| Staking Vault `StakingverseVault.sol` Implementation (upgraded) | [`0x1711b2e1b64F38ca33E51b717CFd27ACD1bd2E2D`](https://explorer.lukso.network/address/0x1711b2e1b64F38ca33E51b717CFd27ACD1bd2E2D?tab=contract) | |
| SLYX Token Proxy | [`0x8a3982f0a7d154d11a5f43eec7f50e52ebbc8f7d`](https://explorer.lukso.network/address/0x8a3982f0a7d154d11a5f43eec7f50e52ebbc8f7d?tab=contract) |
| SLYX Token Implementation | _To be deployed_ |
| SLYX Token Implementation | [`0x08b28405A11348745A3187De2A29C730C53EB29B`](https://explorer.lukso.network/address/0x08b28405A11348745A3187De2A29C730C53EB29B?tab=contract) |

- [Stakingverse Contracts](#stakingverse-contracts)
- [Installation](#installation)
Expand Down
6 changes: 5 additions & 1 deletion audits/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,8 @@ Previous audits for this Vault contract can be found in the [UniversalPage contr

<br>

- [MiloTruck Audit](Stakingverse_Audit_MiloTruck.pdf) - Audit of both StakingverseVault.sol and Liquid Staking contracts
- [MiloTruck Audit](Stakingverse_Audit_MiloTruck.pdf) - Audit of both StakingverseVault.sol and Liquid Staking contracts

<br>

- [Nethermind AI Agent Audit](nethermind-ai-agent-audit-report-stakingverse-pool-contracts.pdf) - Audit of both StakingverseVault.sol and Liquid Staking contracts by the BETA version of the [Nethermind](https://www.nethermind.io) Audit Agent. Check the overview of the findings [here](nethermind-findings.md).
Binary file added audits/nethermind-ai-agent-audit-report-stakingverse-pool-contracts.pdf
View file
Open in desktop
Binary file not shown.
32 changes: 32 additions & 0 deletions audits/nethermind-findings.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@

| Severity | Number |
| ---------- | ------ |
| **High** | 1 |
| **Medium** | 5 |
| **Low** | 12 |
| **Info** | 3 |
| **TOTAL** | 21 |

| # | Severity | Contract | Issue Description | Risk Level | Impact | Resolution |
| ---- | -------- | ------------------------------------- | ------------------------------------------------------------------------ | ---------- | ------------------------------------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| H-1 | High | StakingverseVault.sol | First depositor exploit due to flawed share calculation | High | Share price inflation | ✅ Fixed. Same finding than in MiloTruck audit. Commented in the source code as warning. This contract is used for an upgrade so it will not apply. |
| M-1 | Medium | StakingverseVault.sol | Single oracle has unilateral power to register validators and rebalance | Medium | Potential manipulation of vault balance | ☑️ Acknowledged. Design decision. There are no single oracle that have unilateral power. Oracles are controlled by the admin. If an oracle address is compromised, the operator can register another oracle address and remove the compromised one. |
| M-2 | Medium | StakingverseVault.sol | Insufficient validation in validator registration | Medium | Incorrect validator registrations | ⚪️ Not Applicable / False Positive. Deposit contract on mainnet perform these checks already. https://explorer.lukso.network/address/0xCAfe00000000000000000000000000000000CAfe?tab=contract |
| M-3 | Medium | StakingverseVault.sol | Pending withdrawal rebalancing vulnerability | Medium | Manipulated withdrawal state | ☑️ Acknowledged. The oracle of Stakingverse can be trusted. |
| M-4 | Medium | SLYXToken.sol | Reentrancy risk in `_afterTokenTransfer` during burn | Medium | Possible reentrancy attack | ✅ Mitigated. The `transferStake(...)` function in the `StakingverseVault` contract contains a `nonReentrant` modifier, which mitigates this issue. |
| M-5 | Medium | StakingverseVault.sol | Complex rebalance logic vulnerability | Medium | Misallocation of rewards or fees | ⚪️ Not Applicable / False Positive? Not clear, the issue description seems very broad and not clear to me |
| L-1 | Low | SLYXToken.sol | Use of TransparentUpgradeableProxy assumptions in `_beforeTokenTransfer` | Low | Possible failure on upgrade | ✅ Fixed. The `implementation()` function can only be called by the proxy admin, which would lead to this internal call to fail. This check was removed. |
| L-2 | Low | StakingverseVault.sol | Operators can adjust fee and deposit limit without timelock | Low | Centralized control risk | ☑️ Acknowledged. Operator is controlled mainly by Stakingverse. Would not be the case that drastic fee changes occur out of the sudden. If fee changes, these are communicated to the communities. |
| L-3 | Low | StakingverseVault.sol | Math precision loss in share calculations | Low | Inaccurate balance calculations | ☑️ Acknowledged. We know already and cannot really change much in the logic because it is already deployed and upgrading could incur risks. |
| L-4 | Low | StakingverseVault.sol | Missing zero transfer check in `transferStake()` | Low | Unnecessary gas costs | ☑️ Acknowledged. |
| L-5 | Low | StakingverseVault.sol | Possible rounding issues in fee calculation | Low | Loss of fees due to rounding down | ☑️ Acknowledged. |
| L-6 | Low | SLYXToken.sol | Unprotected initialization parameters | Low | Risk of invalid owner initialization | ⚪️ Not Applicable / False Positive. Check for `address(0)` is performed in the inheritance of the contract. |
| L-7 | Low | SLYXToken.sol | Incomplete error handling in LSP1 Universal Receiver | Low | Potential failures not caught | ⚪️ Not Applicable / False Positive. The description of the issue is not clear. |
| L-8 | Low | StakingverseVault.sol | Missing fee recipient validation in `setFee` | Low | Locked fees without recipient | ⚪️ Not Applicable / False Positive. |
| L-9 | Low | StakingverseVault.sol | No validation for validator existence in `registerValidator` | Low | Easier for malicious oracle to register fake validators | ☑️ Acknowledged. The oracle of Stakingverse can be trusted. |
| L-10 | Low | StakingverseVault.sol | Lack of address validation in `transferStake` | Low | Possible silent failures | ☑️ Acknowledged. Intended behaviour. |
| L-11 | Low | StakingverseVault.sol | Zero value asset loss risk | Low | Incorrect share price calculations | ☑️ Acknowledged. Cannot be applicable as contract is already live and this is an upgrade. |
| L-12 | Low | SLYXToken.sol | Division-by-zero risk in `onVaultStakeReceived` | Low | Possible denial of service | ⚪️ Not Applicable / False Positive. The Vault is already live in production. It is very unlikely that `totalAssets()` will return zero. |
| I-1 | Info | StakingverseVault.sol / SLYXToken.sol | Vault logic failure if proxy pattern changes | Info | Risk in future upgrades | ☑️ Acknowledged. Upgrade will be carefully reviewed including storage layout and the proxy pattern will not be changed (only storage layout will be used). |
| I-2 | Info | SLYXToken.sol | Missing events for critical state changes | Info | Harder to track contract state | ⚪️ Not Applicable / False Positive. Events are emitted on the parent contract in the underlying functions. |
| I-3 | Info | StakingverseVault.sol / SLYXToken.sol | Missing events for critical operations | Info | Difficult off-chain monitoring | ⚪️ Not Applicable / False Positive. Events are emitted on the parent contract in the underlying functions. |
17 changes: 0 additions & 17 deletions output.log
View file
Open in desktop

This file was deleted.

Loading
Loading