GoGoPool contest - 0xNazgul's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L120

Vulnerability details

Impact

ERC4626Upgradeable is an upgradeable version of Solmate's ERC4626 Token. Solmate's convertToShares function follow the formula: assetDepositAmount * totalShareSupply / assetBalanceBeforeDeposit.

The share price always return 1:1 with asset token. If everything work normally, share price will slowly increase with time to 1:2 or 1:10 as more rewards coming in.

But right after xERC4626 contract creation, during first cycle, any user can deposit 1 share set totalSupply = 1. And transfer token to vault to inflate totalAssets() before rewards kick in. (Basically, pretend rewards themselves before anyone can deposit in to get much better share price.)

This can inflate base share price as high as 1:1e18 early on, which force all subsequence deposit to use this share price as base.

Proof of Concept

New ERC4626Upgradeable vault share price can be manipulated right after creation. Which gives early depositor greater share portion of the vault during the first cycle.

While deposit token also affected by rounding precision (due to exploit above) that always return lesser amount of share for user.

Tools Used

Manual Review

Recommended Mitigation Steps

This exploit is unique to contract similar to ERC4626. It only works if starting supply equal 0 or very small number and rewards cycle is very short. Or everyone withdraws, total share supply becomes 0.

This can be easily fix by making sure someone always deposited first so totalSupply become high enough that this exploit become irrelevant. Unless in unlikely case someone made arbitrage bot watching vault factory contract. Just force deposit early token during vault construction as last resort.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/tokens/TokenggAVAX.sol#L88

Vulnerability details

Impact

In the current rewards accounting, vault shares in depositFromStaking() && depositAVAX() && redeemAVAX() can not correctly record the spot yields generated by the staked asset. Yields are released over the next rewards cycle. As a result, malicious users can steal yields from innocent users by picking special timing to depositFromStaking() && depositAVAX() && redeemAVAX().

Proof of Concept

This is a very similar issue to what Frax experienced with a lot of different attack scenarios all with the same underlying root cause in syncRewards().

Tools Used

Manual Review

Recommended Mitigation Steps

Consider following the same recommendation stated by the wardens:

"For the lastRewardAmount not released, allow the users to redeem as it is linearly released later. For the accumulated yields, only allow users to redeem the yields received after 1 rewards cycle after the deposit."

Or what the Frax team decided on:

"syncRewards() should be called by us at the beginning of each period, or we need to automatically call it before deposits/withdrawals."