Yieldy contest - 0x52's results

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L674-L696

Vulnerability details

Impact

User is able to withdraw unstaked asset sooner than they should be

Proof of Concept

Unstake() allows the user to bypass the rebase() call by setting _trigger to false. Since rebase() is bypassed, epoch.number could potentially be stale i.e. doesn't match the Tokemak epoch. A user could potentially call unstake() with _trigger = false immediately after an epoch has ended but expiry would be set using the stale epoch.number because it wouldn't be updated by rebase(). This would allow the user to withdraw early before their funds were actually available in the contract because their withdrawal would be considered to be in the epoch before they actually initiated the withdrawal.

Tools Used

Recommended Mitigation Steps

Rebase() cannot be optional when calling unstake

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L384-L399

Vulnerability details

Impact

Some user withdrawals won't be available for withdrawal even though it should be

Proof of Concept

sendWithdrawalRequest can only happen once per cycle. canBatchTransactions (L386) must return true for the actual withdrawal request to happen. It checks in L362 that currentCycleIndex > lastTokeCycleIndex and in L397 of sendWithdrawlRequest, lastTokeCycleIndex = currentCycleIndex. This means that for the rest of the cycle, canBatchTransactions will return false. This means that if a user requests a withdrawal towards the end of epoch after the withdrawal has been submitted but before the end of the epoch, their withdrawal will be set to the current epoch but the actual token amount of their withdrawal won't be processed. This will lead to a discrepancy between the number of tokens withdrawn and the number of tokens allowed to be withdrawn by users, which means that not all users who are "eligible" for withdrawal will actually be able to withdraw because there won't be enough tokens for everyone.

Tools Used

Recommended Mitigation Steps

The requirement in L362 should be removed. As noted in the contract, "TOKE's requestWithdrawal overwriting the amount if you call it more than once per cycle". Overwriting the withdrawal is perfectly okay though because it already uses requestWithdrawalAmount which is a cumulative measure of the tokens that need to be withdrawn because it is only decreased when the asset it actually received from a withdrawal

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L517-L565

Vulnerability details

Impact

Rebases can be frontrun with very little token downtime even when warmUpPeriod > 0

Proof of Concept

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L415-L417

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L703

A user can call stake the block before epoch.endTime <= block.timestamp, allowing the user to bypass the forced rebase called in L416 of the the stake function. If warmUpPeriod > 0 then the user will receive a "warmUpInfo" with the value of their deposit. The very next block, the user can then call instantUnstakeCurve.

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Staking.sol#L600-L627

This will call rebase again in L633 and this time epoch.endTime <= block.timestamp will be true and it will trigger an actual rebase, distributing the pending rewards. _retrieveBalanceFromUser (L617) will then allows the user to unstake all the funds locked in warm up. The issue is that when unstaking it uses userWarmInfo.credits meaning that any rebalance rewards are kept. This allows the user to get in, collect the rebase, then immediately get out.

Tools Used

Recommended Mitigation Steps

Being able to unstake tokens even when in the warm up period is a useful feature but tokens unstaked during that period should not be allowed to accumulate any rebases or it can lead to situations like this. L537-L539 should be changed to: warmUpBalance = userWarmInfo.amount

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Migration.sol#L43-L58

Vulnerability details

Impact

Users of moveFundsToUpgradedContract() in migration.sol may forfeit rebase rewards

Proof of Concept

L54 calls instantUnstake(false) meaning that it skips the optional rebase. If there is a pending rebase then the user calling the function will not get this rebase and miss out on potential rewards.

Tools Used

Recommended Mitigation Steps

Add an input bool _trigger then add the following code to the start of the function:

if (_trigger) { rebase(); }

This allows users to optionally call rebase if they are concerned about losing pending rebase rewards

Lines of code

https://github.com/code-423n4/2022-06-yieldy/blob/524f3b83522125fb7d4677fa7a7e5ba5a2c0fe67/src/contracts/Migration.sol#L43-L58

Vulnerability details

Impact

moveFundsToUpgradedContract() will fail because of fee

Proof of Concept

In L54 the function uses instantUnstake to unstake but it doesn't account for the fees taken by it. Because of this fee, it won't receive the full amount and then staking in L56 will fail because it tries to call the balance before fees

Tools Used

Recommended Mitigation Steps

Take the balance of the token before and after instantUnstake to figure out how much is actually unstaked then call L56 with that update balance