GoGoPool contest - Breeje's results

Lines of code

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

Vulnerability details

convertToShares function follow the formula:

  return supply == 0 ? assets : assets.mulDivDown(supply, totalAssets());

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.

Proof of Concept

Right after TokenggAVAX 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.

Impact

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

Recommended Mitigation Steps

This exploit is unique to contract similar to ERC4626. It only works if starting supply equals very small number and rewards cycle is very short. Or everyone withdraws, total share supply become close to 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/upgradeable/ERC4626Upgradeable.sol#L42-L54 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L91-L112 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/tokens/TokenggAVAX.sol#L88-L130

Vulnerability details

In the current rewards accounting, vault shares in deposit() and redeem() 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 deposit() and redeem().

If withdraw just after the rewardsCycleEnd timestamp, a user can not get the yields from last rewards cycle. Since the totalAssets() only contain totalReleasedAssets but not the yields part. It takes 1 rewards cycle to linearly add to the totalReleasedAssets.

Proof of Concept

Assume per 10,000 asset staking generate yields of 70 for 7 days, and the reward cycle is 1 day. A malicious user Alice can do the following:

• Watch the mempool for withdraw(10,000) from account Bob, front run it with syncRewards(), so that the most recent yields of amount 70 from Bob will stay in the vault.
• Alice will also deposit a 10,000 to take as much shares as possible.
• After 1 rewards cycle of 1 day, redeem() to take the yields of 70.

Effectively steal the yields from Bob. The profit for Alice is not 70, because after 1 day, her own deposit also generates some yield, in this example this portion is 1. At the end, Alice steal yield of amount 60.

Recommended Mitigation Steps

• For the lastRewardsAmt not released, allow the users to redeem as it is linearly released later.

Lines of code

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

Vulnerability details

totalReleasedAssets is a cached value of total assets which will only include the unlockedRewards when the whole cycle ends.

This makes it possible for totalReleasedAssets -= amount to revert when the withdrawal amount exceeds totalReleasedAssets, as the withdrawal amount may include part of the unlockedRewards in the current cycle.

Proof of Concept

Given:

• rewardsCycleLength = 14 days
• Alice deposit() 100 AVAX tokens
• 5 AVAX tokens are transferred as rewards and syncRewards() was called
• 1 day later, Alice redeem() with all shares, the transaction will revert at beforeWithdraw().

Alice’s shares worth 105 AVAX at this moment, but totalReleasedAssets = 100, making totalReleasedAssets -= amount reverts due to underflow.

Bob deposit() 5 AVAX tokens;

• Alice withdraw() 105 AVAX tokens, totalReleasedAssets becomes 0;
• Bob can’t even withdraw 1 wei of AVAX token, as totalReleasedAssets is now 0.
• If there are no new deposits, both Alice and Bob won’t be able to withdraw any of their funds until rewardsCycleEnd

Recommended Mitigation Steps

Consider changing to:

File: contract/tokens/TokenggAVAX.sol

  function beforeWithdraw(
		uint256 amount,
		uint256 /* shares */
	) internal override {
    uint256 _totalReleasedAssets = totalReleasedAssets;
    if (amount >= _totalReleasedAssets) {
        uint256 _totalAssets = totalAssets();
        lastRewardAmount -= _totalAssets - _totalReleasedAssets;
        lastSync = block.timestamp;
        totalReleasedAssets = _totalAssets - amount;
    } else {
        totalReleasedAssets = _storedTotalAssets - amount;
    }
  }

Link to code