Illuvium (Part 4 - Staking V2)

Quantstamp17 Mar 2022Files named in the report

Quantstamp reviewed Illuvium (Illuvium (Part 4 - Staking V2)) on 17 Mar 2022: 19 findings, 0 critical; 15 marked fixed, 4 acknowledged.

No open critical findings

Open the report Open Illuvium’s security record

Report hosted on certificate.quantstamp.com.

Trustblock does not review code.

Findings19

Statuses are the security firm’s own, from its report.

Severity	Acknowledged	Fixed
Critical	0	0
High	1	1
Medium	0	2
Low	3	12

HighMissing function callFixed

The comment on the following line is not implemented; only one value (`subVaultRewards `) is updated. ``` // subYieldRewards and subVaultRewards needs to be updated on every `_processRewards` call user.subVaultRewards = uint256(user.totalWeight).weightToReward(vaultRewardsPerWeight); ```

MediumUnchecked Return ValuesFixed

The function `stake` calls `transferFrom` but ignores the return value; function `unstake` calls `transfer` and also ignores the return value.

MediumUnlimited ApprovalsFixed

In `sendILVRewards`, there are approvals with `type(uint256).max` which is dangerous if something goes wrong.

LowEvent `LogMigrateUser` Could Present More InformationFixed

The function `migrateUser(...)` contains the following: `emit LogMigrateUser(msg.sender, _to)`. Perhaps adding more information to this log would help in future investigations.

LowExternal Functions of `ILVPool.sol` can be Called by AnyoneFixed

The functions from this file can be called by anyone. However, the code comment states that “External `MigrateLockedStakes` call, [is] used in Sushi LP pool”.

LowIncorrect `paramIndex`Fixed

In `CorePool.sol`, lines 274 and 275 `paramIndex` for `_initTime` and `_weight` are off-by-one. Similarly, line 40 in `VaultRecipient.sol` has an incorrect `paramIndex`.

LowIncorrect and Unclear `Keccak256`Fixed

In `ILVPool.sol`, the hash comment `bytes4(keccak256("_migrateYieldWeights(bytes32[],uint256,uint256")))` in function `_migrateYieldWeights` does not have the closing bracket for `_migrateYieldWeights` inside the quote. In `CorePool.sol`, the hash `0x867a0347` is not explained when it should be.

LowIndistinguishable Return ValueFixed

In `getV1StakePosition`, the value zero is returned if the stake id provided is not found. However, this value is also returned if the stake is the first entry.

LowOwnership can be RenouncedFixed

All files that inherit from the `Ownable` contract could be left with no owner if the latter renounces his ownership. As one potential consequence, that could prevents update to the deployed contracts, requiring re-deploys and potential downtime.

LowPoorly Named Function: `CorePool.migrateUser(…)`Fixed

The function `migrateUser(...)` is related to migrations of wallets of the same user, and not migrating users from V1 to V2. Thus, we encourage the team to provide a better name for this function.

Show 5 more

LowReview the Visibility of State VariablesFixed

The proper declaration of the visibility of state variables is a good programming practice and it also reduces the final contract size. Public variables increase the contract size:for each public variable, the solidity compiler automatically generates the `getter` and `setter`[functions](https://docs.soliditylang.org/en/v0.6.10/contracts.html#visibility-and-getters). For instance, we do not see a reason for declaring the following state variables as `public` in `Vault.sol`: - `Pools public pools;` - `IUniswapV2Router02 public sushiRouter` and - `IERC20 public ilv`.

LowState Variables can Change when the Contract is PausedFixed

The functions `migrateUser(...)`, `sync()`, `receiveVaultRewards(...)`, `setWeight(...)`, and `pendingRewards(...)`can be called when the contract is paused. This can be dangerous since they change state variables.

LowSuboptimal CodeFixed

The contract size can be optimized by turning `modifiers` into `functions`, as shown in the example below: ``` modifier checkStuff() {} function doSomething() checkStuff {} ``` The `function doSomething()` can rewritten without modifiers as: ``` function checkStuff() private {} function doSomething() { checkStuff(); } ```

LowUnrestricted `endTime`Fixed

In `setEndTime`, there is no sanity checks for the new value. It could be before the current time, or interact with existing pools in unexpected ways.

LowUnsafe CastFixed

The function `unstake` in `FlashPool.sol` converts a `uint256` to a `unit128` without checking that the cast is safe. Similar issues - with as small a bit width as 64 - exist elsewhere in the contract. These issues are found in `CorePool.sol` as well.