Yieldy contest - Lambda's results

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/34774d3f5e9275978621fd20af4fe466d195a88b/src/contracts/Staking.sol#L308

Vulnerability details

Impact

The withdraw function of the ETH Tokemak pool has an additional parameter asEth. This can be seen in the Tokemak Github repository or also when looking at the deployed code of the ETH pool. Compare that to e.g. the USDC pool, which does not have this parameter.

This means that the call to withdraw will when the staking token is ETH / WETH and no withdrawals would be possible.

Proof of Concept

A new Staking contract with ETH / WETH as the staking token is deployed. Deposits in Tokemak work fine, so users stake their tokens. However, because of the previously described issue, no withdrawal is possible, leaving the funds locked.

Recommended Mitigation Steps

Handle the case where the underlying asset is WETH / ETH separately and pass this boolean in that case.

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/34774d3f5e9275978621fd20af4fe466d195a88b/src/contracts/BatchRequests.sol#L37 https://github.com/code-423n4/2022-06-yieldy/blob/34774d3f5e9275978621fd20af4fe466d195a88b/src/contracts/BatchRequests.sol#L93

Vulnerability details

Impact

When contracts[i] is the 0 address (which happens after removeAddress was called), IStaking(contracts[i]).canBatchTransactions() will fail for this index, meaning that the whole function will always fail.

Recommended Mitigation Steps

Check if the address is equal to 0.

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/34774d3f5e9275978621fd20af4fe466d195a88b/src/contracts/Staking.sol#L98

Vulnerability details

Impact

In the constructor of Staking.sol, it is not enforced that the _firstEpochEndTime is larger than the current block.timestamp. If a low value is accidentally passed (or even 0), rebase can be called multiple times in sucession, causing the epoch.number to increase. Therefore, the coolDown & warmUp period can be circumvented in such a scenario, as epoch.number >= info.expiry (in _isClaimAvailable and _isClaimWithdrawAvailable) will return true after rebase caused several increases of epoch.number.

Recommended Mitigation Steps

Either require that _firstEpochEndTime is larger than block.timestamp or set the expiry of the first epoch to block.timestamp + _epochDuration.

• The comment in Line 323 of Staking.sol is wrong, which can be confusing. In the case of unstakeAllFromTokemak, we have per definition balance == _amount (as the balance of the pool is passed from unstakeAllFromTokemak).
• The variable name in Line 112 of LiquidityReserve.sol still refers to the old project (FOX).
• Hardcoding the COW contract addresses in Line 73 and Line 74 of Staking.sol goes against best practices.
• In Line 88 of Staking.sol, the code from line 84 is duplicated, i.e. the yieldy token approval for the liquidity reserve is executed two times.
• Duplicated code: In Line 144 and Line 372 of Staking.sol, the function _getTokemakBalance could be used.
• amountToRequest should be passed to requestWithdrawal in Line 326 of Staking.sol, otherwise the wrong value is used when balance < _amount.
• In Line 372, tokePoolContract was already instantiated with ITokePool, there is no reason to call ITokePool(tokePoolContract) instead of tokePoolContract.balanceOf
• In Line 100 of Yieldy.sol, the new supply is passed as _previousCirculating to _storeRebase. In Line 121, this leads to totalStakedBefore and totalStakedAfter always being equal (and a wrong rebasePercent, which is also calculated with the _previousCirculating.
• The creditAmount calculation in Yieldy.sol is inconsistent. In transferFrom, creditsForTokenBalance is called, whereas the calculation is done directly in transfer, _mint, and _burn. Consider using one consistent method for that.
• To have a consistent behavior with rebase, it should be require(_totalSupply <= MAX_SUPPLY, ...); in Line 257 of Yieldy.sol, i.e. _totalSupply == MAX_SUPPLY should also be allowed.

• IERC20Upgradeable(TOKE_TOKEN) can be saved in a variable to reduce the number of memory reads by one in transferToke.
• In _storeRebase within Yieldy.sol, _totalSupply is used two times (line 122 and line 129) and can therefore be stored to a variable to save one read.
• In Line 93 of BatchRequests.sol, the array size is never decreased when an item is deleted. Therefore, the other functions of the contract also need to iterate over deleted entries, which can incur a lot of additional gas. Consider decreasing the array size (combined with a swap of the last array entry).
• In Line 321, tokePoolContract can be used instead of reading TOKE_POOL again.
• The _isClaimAvailable check in Line 428 is also performed within claim, i.e. it is unnecessary performed two times, which costs gas.
• The following subtractions can be unchecked, because underflow is already checked before:
  • https://github.com/code-423n4/2022-06-yieldy/blob/34774d3f5e9275978621fd20af4fe466d195a88b/src/contracts/Staking.sol#L716
  • https://github.com/code-423n4/2022-06-yieldy/blob/8400e637d9259b7917bde259a5a2fbbeb5946d45/src/contracts/Yieldy.sol#L212
  • https://github.com/code-423n4/2022-06-yieldy/blob/8400e637d9259b7917bde259a5a2fbbeb5946d45/src/contracts/Yieldy.sol#L192
  • https://github.com/code-423n4/2022-06-yieldy/blob/8400e637d9259b7917bde259a5a2fbbeb5946d45/src/contracts/Yieldy.sol#L288
• In Line 288 of Yieldy.sol, currentCredits can be used instead of reading it again.
• In Line 83 of Yieldy.sol, there is an unnecessary storage read, currentTotalSupply could be used instead.