GoGoPool contest - fs0c's results

Lines of code

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

Vulnerability details

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 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

The share price can be manipulated and due to rounding precision other users will recieve less (or zero) shares when they deposit asset.

POC

function testinflate() public{
		uint depositamount = 1 ether;
		vm.deal(bob,depositamount);
		vm.startPrank(bob);
		wavax.deposit{value: depositamount}();
		wavax.transfer(address(ggAVAX),depositamount);
		
		vm.deal(bob,1);
		ggAVAX.depositAVAX{value: 1}();
		skip(14 days);
		ggAVAX.syncRewards();
		skip(14 days);

		uint sharestoasset = ggAVAX.convertToAssets(1);
		emit log_named_decimal_uint("1 share to asset", sharestoasset,18 );
	}

Recommendations

More about the similar issue here: https://rokinot.github.io/hatsfinance

Lines of code

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

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().

POC

Assume that 10000 asset staking generate yields of 100 in 14 days, and the reward cycle is also 14 days. And a user bob has put 10000 for 28 days, which should generate rewards of 200 tokens.

• Let’s say bob will call withdrawal after 28 days and is expecting 200 tokens. Alice the attacker will watch mempool for withdrawal from bob and front run it with syncRewards , so the most recent yield of 200 tokens will remain in the vault and bob will get very less rewards as the rewardsCycleEnd is updated and the code will run into the following lines:

  uint256 unlockedRewards = (lastRewardsAmt_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_);

• Alice will also deposit 10000 tokens and wait for reward cycle to end and call redeem to take the 200 tokens.

Another case:

When the Multisig Treasury transfers new yields into the vault, the new yields will accumulate until syncRewards() is called. It is possible that yields from multiple rewards cycles accumulates, and being released in the next cycle.

Knowing that the yields has been accumulated for 3 rewards cycles, a malicious user can deposit() and call syncRewards() to trigger the release of the rewards. redeem() after 1 cycle.

Here the malicious user gets yields of 3 cycles, lose 1 in the waiting cycle. The net profit is 2 cycle yields, and the gained yields should belong to the other users in the vault.

Recommendations

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

More about a similar issue here : https://github.com/code-423n4/2022-09-frax-findings/issues/110

Lines of code

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

Vulnerability details

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

function beforeWithdraw(
		uint256 amount,
		uint256 /* shares */
	) internal override {
		totalReleasedAssets -= amount;
	}

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

function syncRewards() public {
		uint32 timestamp = block.timestamp.safeCastTo32();

		if (timestamp < rewardsCycleEnd) {
			revert SyncError();
		}

		uint192 lastRewardsAmt_ = lastRewardsAmt;
		uint256 totalReleasedAssets_ = totalReleasedAssets;
		uint256 stakingTotalAssets_ = stakingTotalAssets;

		uint256 nextRewardsAmt = (asset.balanceOf(address(this)) + stakingTotalAssets_) - totalReleasedAssets_ - lastRewardsAmt_;

		// Ensure nextRewardsCycleEnd will be evenly divisible by `rewardsCycleLength`.
		uint32 nextRewardsCycleEnd = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;

		lastRewardsAmt = nextRewardsAmt.safeCastTo192();
		lastSync = timestamp;
		rewardsCycleEnd = nextRewardsCycleEnd;
		totalReleasedAssets = totalReleasedAssets_ + lastRewardsAmt_;
		emit NewRewardsCycle(nextRewardsCycleEnd, nextRewardsAmt);
	}

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 unlockedRewardsin the current cycle.

POC

1. Alice deposits 14e18 WAVAX using depositAVAX.
2. Let’s assume that the minipool manager transfers 14e18 which were earned as rewards and calls syncRewards.
3. 1 day later Alice tries to run redeemAVAX with all the shares but transaction will be reverted at beforeWithdraw.

Alice’s shares are worth 15e18 at this moment, but totalReleasedAssets = 14e18 so the underflow happens.

function testinit() public {
		uint256 depositAmount = 14 ether;
		vm.deal(bob,depositAmount);
		vm.prank(bob);
		ggAVAX.depositAVAX{value: depositAmount}();

		assertEq(bob.balance, 0);
		assertEq(wavax.balanceOf(address(ggAVAX)), depositAmount);
		assertEq(ggAVAX.balanceOf(bob), depositAmount);
		assertEq(ggAVAX.convertToShares(ggAVAX.balanceOf(bob)), depositAmount);

		uint256 rewards = 14 ether;
		wavax.mint(address(ggAVAX),rewards); // assume rewards deposited here
		skip(14 days);
		ggAVAX.syncRewards();
		skip(1 days);
		emit log_uint(ggAVAX.totalAssets());

		vm.prank(bob);
		ggAVAX.redeemAVAX(depositAmount); // will revert because of underflow
	}

Recommendation

The bug is exactly similar to https://code4rena.com/reports/2022-04-xtribe/#m-01-xerc4626sol-some-users-may-not-be-able-to-withdraw-until-rewardscycleend-the-due-to-underflow-in-beforewithdraw so the same recommendation should work here as well.