GoGoPool contest - btk's results

Lines of code

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

Vulnerability details

Impact

ERC4626 vault share price can be maliciously inflated on the initial deposit, leading to the next depositor losing assets due to precision issues.

Proof of Concept

The first depositor of an ERC4626 vault can maliciously manipulate the share price by depositing the lowest possible amount (1 wei) of liquidity and then artificially inflating ERC4626.totalAssets.

This can inflate the base share price as high as 1:1e18 early on, which force all subsequence deposit to use this share price as a base and worst case, due to rounding down, if this malicious initial deposit front-run someone else depositing, this depositor will receive 0 shares and lost his deposited assets.

Given a vault with AVAX as the underlying asset:

Alice (attacker) deposits initial liquidity of 1 wei AVAX via depositAVAX(). Alice receives 1e18 (1 wei) vault shares. Alice transfers 1 ether of AVAX to the vault to artificially inflate the asset balance without minting new shares. The asset balance is now 1 ether + 1 wei AVAX -> vault share price is now very high (= 1000000000000000000001 wei ~ 1000 * 1e18). Bob (victim) deposits 100 ether AVAX. Bob receives 0 shares due to a precision issue. His deposited funds are lost.

The shares are calculated as following:

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

In case of a very high share price, due to totalAssets() > assets * supply, shares will be 0.

Tools Used

Manual Review

Recommended Mitigation Steps

This is a well-known issue, Uniswap and other protocols had similar issues when supply == 0.

For the first deposit, mint a fixed amount of shares, e.g. 10**decimals()

if (supply == 0) {
    return 10**decimals; 
} else {
    return assets.mulDivDown(supply, totalAssets());
}

Judge has assessed an item in Issue #365 as 2 risk. The relevant finding follows:

[L-4] Misleading comments - Multisig are still managing pool

Lines of code

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

Vulnerability details

Impact

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

Proof of Concept

In syncRewards(), the current asset balance is break into 2 parts: totalReleasedAssets and lastRewardsAmt/nextRewardsAmt. The lastRewardsAmt is the surplus balance of the asset, or the most recent yields.

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);
}

And in the next rewards cycle, lastRewardsAmt will be linearly added to totalReleasedAssets, their sum is the return value of totalAssets():

function totalAssets() public view override returns (uint256) {
	// cache global vars
	uint256 totalReleasedAssets_ = totalReleasedAssets;
	uint192 lastRewardsAmt_ = lastRewardsAmt;
	uint32 rewardsCycleEnd_ = rewardsCycleEnd;
	uint32 lastSync_ = lastSync;

	if (block.timestamp >= rewardsCycleEnd_) {
		// no rewards or rewards are fully unlocked
		// entire reward amount is available
		return totalReleasedAssets_ + lastRewardsAmt_;
	}

	// rewards are not fully unlocked
	// return unlocked rewards and stored total
	uint256 unlockedRewards = (lastRewardsAmt_ * (block.timestamp - lastSync_)) / (rewardsCycleEnd_ - lastSync_);
	return totalReleasedAssets_ + unlockedRewards;
}

totalAssets() will be referred when depositAVAX() and redeemAVAX().

Based on the above rules, there are 2 potential abuse cases:

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

2. When the Multisig 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 depositAVAX() and call syncRewards() to trigger the release of the rewards. redeemAVAX() after 1 cycle.

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

Tools Used

Manual Review

Recommended Mitigation Steps

• for the lastRewardsAmt 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.

Lines of code

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

Vulnerability details

Impact

Some users may not be able to withdraw until rewardsCycleEnd the due to underflow in beforeWithdraw()

Proof of Concept

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

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.

A previous report that addressed the same issue is available here:

• https://github.com/code-423n4/2022-09-frax-findings/issues/393

Tools Used

Manual Review

Recommended Mitigation Steps

Consider changing beforeWithdraw() to:

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