GoGoPool contest - ck's results

Lines of code

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

Vulnerability details

Impact

When a user deposits AVAX for staking it is converted to ggAVAX as follows:

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

function convertToShares(uint256 assets) public view virtual returns (uint256) {
		uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

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

In an ideal situation, the first deposits of AVAX will lead to an allocation of an equal number of shares (ggAVAX) because initial supply is 0.

A malicious user could however manipulate the value of totalAssets() which is a sum of AVAX and rewards in the vault. The attacker could first make a deposit of AVAX and get allocated ggAVAX using the current convertToShares calculation. The attacker could then send a small amount of AVAX to the vault which would lead to an increase in the value of totalAssets(). This would make users that make AVAX deposits after that get allocated fewer ggAVAX than if the attacker had not manipulated the value of totalAssets(). During the next reward cycles, the attacker as a result of having an inflated amount of ggAVAX as compared to the other users will get higher rewards.

Proof of Concept

Starting from a reference state where supply = 0 and totalAssets() = 0.

• The attacker deposits 1000 AVAX and gets allocated 1000 ggAVAX because supply = 0.

Current values are now: supply = 1000 and totalAssets() = 1000. If another user deposits 1000 AVAX they should also get 1000 ggAVAX because the calculation is (1000 x 1000) / 1000 = 1000.

• The attacker sends 10 AVAX to the vault to manipulate the share price.

The manipulated values are now: supply = 1000 and totalAssets() = 1010. Now if another user deposits AVAX, they would get 990 ggAVAX as a result of the manipulation: (1000 x 1000) / 1010 = 990.

This could affect a great number of users as they would all be subject to the manipulated share price.

Tools Used

Manual Analysis, Hardhat

Recommended Mitigation Steps

Ensure that the vault is seeded with sufficient AVAX liquidity to make exploitation expensive for an attacker.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L37-L43 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L89-L114

Vulnerability details

Impact

The ClaimNodeOp.sol contract is not included in the important contracts whose actions are restriced by pauseEverything().

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L37-L43

function pauseEverything() external onlyDefender {
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		dao.pauseContract("TokenggAVAX");
		dao.pauseContract("MinipoolManager");
		dao.pauseContract("Staking");
		disableAllMultisigs();
	}

This is despite the ClaimNodeOp.sol contract having the claimAndRestake() function which would allow node operators to claim rewards even when pauseEverything() has been called by Defenders. It would be important to have this functionality pausable especially if there was an exploit that affected rewards.

claimAndRestake(): https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L89-L114

Proof of Concept

Node operators would still be able to claim and withdraw tokens even when Defenders have called pauseEverything() in the Ocyticus.sol contract.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Modify the pauseEverything() function to:

function pauseEverything() external onlyDefender {
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		dao.pauseContract("TokenggAVAX");
		dao.pauseContract("MinipoolManager");
		dao.pauseContract("Staking");
                dao.pauseContract("ClaimNodeOp");
		disableAllMultisigs();
	}

Or add the whenNotPaused modifier to the claimAndRestake() function in the ClaimNodeOp.sol contract.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L547-L551 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L423-L425 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L379-L383 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L94-L97

Vulnerability details

Impact

Detailed description of the impact of this finding.

When recordStakingEnd() is called and there are no AVAX awards due to a validator not meeting requirements, the GGP stake of the node operator is subtracted:

recordStakingEnd(): https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L423-L425

                // No rewards means validation period failed, must slash node ops GGP.
		if (avaxTotalRewardAmt == 0) {
			slash(minipoolIndex);
		}

The amount to be slashed is calculated by determining the value of GGP that is equivalent to the expected AVAX rewards:

calculateGGPSlashAmt(): https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L547-L551

Then the amount is deducted from the node operators GGP stake using slashGGP() and finally decreaseGGPStake().

slashGGP: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L379-L383

decreaseGGPStake: https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L94-L97

function decreaseGGPStake(address stakerAddr, uint256 amount) internal {
		int256 stakerIndex = requireValidStaker(stakerAddr);
		subUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpStaked")), amount);
	}

If the price of GGP in relation to AVAX falls low enough from when a pool was created to the end of the 14 day cycle, the amount of GGP needed to be slashed would be higher than the amount they have staked. This would lead to a revert in the decreaseGGPStake() function. ggAVAX stakers would lose their rewards while the node operator would keep their GGP.

Proof of Concept

Consider the following scenario

A node operator creates a minipool with 1000 AVAX and stakes 100 GGP (Assume price of 1 GGP = 1 AVAX). At the end of the cycle, there are no rewards because the validator didn't meet requirements. Assume that the price of GGP has fallen to 1 GGP = 0.01 AVAX. Assume the expected reward for the period was 10 AVAX. This would be equivalent to 1000 GGP. Attempting to decrease the node operators stake would lead to a revert due to an underflow (100 - 1000). The ggAVAX stakers would therefore lose their rewards.

Tools Used

Manual Analysis

Recommended Mitigation Steps

Consider implementing a check in decreaseGGPStake() to confirm the amount to be deducted is not higher than the GGP Staked. The ggAVAX stakers could be compensated through some of the node operators AVAX in such scenarios if the developers think that is acceptable.

Lines of code

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

Vulnerability details

Impact

When users stake ggAVAX for a reward cycle, the rewards for that cycle are not allocated to only the stakers of that cycle. Instead the rewards will be allocated during the next reward cycle.

This means that new users who deposit AVAX for ggAVAX at the end of a cycle will get streamed rewards from the previous cycle. This will mean that stakers of the previous cycle will share the rewards with stakers of the current cycle leading to loss of reduced rewards.

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

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

Note that from the syncRewards() function, rewards will be streamed after cycle end and will be distributed to all stakers even those who hadn't staked during the current cycle.

Proof of Concept

From a newly cloned repo:

Setup

just node

just evm

Allocate Alice 2000 ggAVAX and ensure Bob has 0 ggAVAX

just task ggavax:liqstaker_redeem_ggavax --actor alice --amt 10000
just task ggavax:liqstaker_deposit_avax --actor alice --amt 2000
                                                                                                            
just task ggavax:liqstaker_redeem_ggavax --actor bob --amt 10000

NodeOp1 creates and launches a minipool

just task staking:stake_ggp --actor nodeOp1 --amt 300
just task minipool:create --actor nodeOp1 --node node1 --duration 14d
just task minipool:list_claimable --actor rialto1
just task minipool:claim --actor rialto1
just task minipool:recordStakingStart --actor rialto1 --node node1
just task minipool:list

Skip to end of cycle and complete the reward cycle

just task debug:skip --duration 14d 
just task minipool:recordStakingEnd --actor rialto1 --node node1 --reward 300

sync rewards for the cycle

just task ggavax:sync_rewards --actor rialto1

At this point only Alice participated in the cycle but rewards will be streamed over the next 14 days. Let say Bob deposits 2000 ggAVAX.

just task ggavax:liqstaker_deposit_avax --actor bob --amt 2000

If we now jump 14 days, Bob will get a share of the previous cycles rewards even though there weren't any active minipools when he joined.

just task debug:skip --duration 14d

just task debug:list_actor_balances

Output

alice      0xC70f1A9B1CBb13C7fF1A8a847f8EF188d89730e0 998000.00000         2000.00000           2063.75944           0.00000             
bob        0xcF14BAa2352770904C2BB17783FFf2a92C48bf7a 998000.00000         1999.98170           2063.74056           0.00000 

It is evident that Alice lost rewards to Bob. Alice joined a cycle earlier than Bob but the way rewards are streamed means that Bob also gets a share of the rewards a cycle later.

Tools Used

Hardhat, Manual Analysis.

Recommended Mitigation Steps

Implement a check of active ggAVAX stakers during a cycle and have rewards for that cycle streamed to them only.

Lines of code

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

Vulnerability details

Impact

In the beforeWithdraw() function, it is possible for totalReleasedAssets -= amount; to revert due to an underflow.

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

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

totalReleasedAssets is calculated as totalReleasedAssets = totalReleasedAssets_ + lastRewardsAmt_; only when the next rewards cycle has ended in the syncRewards() function.

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

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

It is therefore possible for the amount requested for withdrawal to be higher than totalReleasedAssets until the next cycle ends. This means some users will be unable to access their funds for an extended period of time. While some users will be able to withdraw their funds, those who try to withdraw later may be unable to for an extended period.

Proof of Concept

From the a new cloned instance from Github - https://github.com/code-423n4/2022-12-gogopool

Initial setup

just node

just setup-evm

Start off on a clean slate of ggAVAX balances

just task ggavax:liqstaker_redeem_ggavax --actor alice --amt 10000
just task ggavax:liqstaker_redeem_ggavax --actor bob --amt 10000

Then assign Alice 1000 ggAVAX and Bob 200 ggAVAX

just task ggavax:liqstaker_deposit_avax --actor alice --amt 1000 
just task ggavax:liqstaker_deposit_avax --actor bob --amt 200 

nodeOp1 creates a minipool and runs it for 14days.

just task staking:stake_ggp --actor nodeOp1 --amt 300
just task minipool:create --actor nodeOp1 --node node1 --duration 14d
just task minipool:list_claimable --actor rialto1
just task minipool:claim --actor rialto1
just task minipool:recordStakingStart --actor rialto1 --node node1

Skip to the end of the cycle.

just task debug:skip --duration 14d

The minipool has now ended and the pools funds are withdrawn

just task minipool:recordStakingEnd --actor rialto1 --node node1 --reward 300
just task minipool:withdrawMinipoolFunds --actor nodeOp1 --node node1

sync ggAVAX awards

just task ggavax:sync_rewards --actor rialto1

At this stage, the 14 days period has ended and the rewards distributed. The problem is that the calculation totalReleasedAssets = totalReleasedAssets_ + lastRewardsAmt_ will not be done with the rewards until after the next 14 days.

It then becomes a race between Alice and Bob on who redeems their ggAVAX rewards first. The one who does it first will get their AVAX rewards while the other will have to wait till the next 14 days because the totalReleasedAssets will have fallen below the amount they are owed until the next rewards cycle passes.

Let's say Alice withdraws first:

just task ggavax:liqstaker_redeem_ggavax --actor alice --amt 1000

After she withdraws totalReleasedAssets will fall below 200 which Bob is currently owed.

just task debug:list_vars

Notice that totalReleasedAssets is now below the 200 ggAVAX that Bob should be able to redeem.

ggAVAX Variables:
rwdCycEnd       lstRwdAmt       totRelAss       stakTotAss      AmtAvlStak      totAssets      
1674086400      127.50000       199.87820       0.00000         180.02192       200.02436 

At attempt at withdrawal will result in a revert.

just task ggavax:liqstaker_redeem_ggavax --actor bob --amt 200

Revert Output

npx hardhat ggavax:liqstaker_redeem_ggavax --actor bob --amt 200
redeeming for 200.006281012817112698 avax
An unexpected error occurred:

ProviderError: Error: Transaction reverted without a reason string

This means while Alice was successful in redemption, Bob has to wait another 14 days and hope the issue does not reoccur.

This issue can reoccur even at the subsequent cycle leaving Bob and some other users with their funds locked up for an extended period.

Tools Used

Hardhat

Recommended Mitigation Steps

A possible solution would be to ensure that the contract has surplus AVAX between cycles to ensure withdrawals can be met.