GoGoPool contest - koxuan's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/tokens/TokenggAVAX.sol#L166-L178 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/tokens/TokenggAVAX.sol#L225-L227 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L132-L134 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L120-L124

Vulnerability details

Impact

First depositor for TokenggAVAX can mint one share of token, transfer a large amount of AVAX to TokenggAVAX to inflate the price of one share, causing future depositor to lose out on precision loss when they deposit.

Proof of Concept

First depositor will call depositAVAX with 1 AVAX, this will mint 1 share to user. The price of 1 share is 1 AVAX.

	function depositAVAX() public payable returns (uint256 shares) {
		uint256 assets = msg.value;
		// Check for rounding error since we round down in previewDeposit.
		if ((shares = previewDeposit(assets)) == 0) {
			revert ZeroShares();
		}

		emit Deposit(msg.sender, msg.sender, assets, shares);

		IWAVAX(address(asset)).deposit{value: assets}();
		_mint(msg.sender, shares);
		afterDeposit(assets, shares);
	}

The user then transfers large amount of AVAX, 15000 AVAX over. The price of 1 share is now 15001 AVAX. The second depositor deposits 20000 AVAX. He receives 1 share.

Now first depositor withdraws his 1 share. With only 2 shares and 35001 AVAX in the vault, first depositor can withdraw 17500. He gains 2499 AVAX from second depositor.

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend minimum amount of share to be minted if vault has no share and also burn away a portion of the first depositor share so that the price of share is more resilient to such attack.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L385-L440 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L424-L426 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L670-L683 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Staking.sol#L379-L383 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Staking.sol#L94-L97 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/BaseAbstract.sol#L195-L197 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/Storage.sol#L176-L178

Vulnerability details

Impact

ggp is staked by node operator as a collateral for liquid stakers avax. However, there are conditions whereby slash will revert and therefore causing recordStakeEnd to revert too in the event that rialto wants to slash the node operator for bad behaviour. The funds will be stuck and there is no way forward unless rialto decides to reward node operator even though they are meant to be slashed.

Proof of Concept

When rialto decides to slash a node operator, they call recordStakingEnd with 0 avaxTotalRewardAmt. This will call slash which will call staking's slashGGP. Order of function calls, recordStakingEnd -> slash -> slashGGP.

		if (avaxTotalRewardAmt == 0) {
			slash(minipoolIndex);
		}

	function slash(int256 index) private {
		address nodeID = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".nodeID")));
		address owner = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".owner")));
		uint256 duration = getUint(keccak256(abi.encodePacked("minipool.item", index, ".duration")));
		uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt")));
		uint256 expectedAVAXRewardsAmt = getExpectedAVAXRewardsAmt(duration, avaxLiquidStakerAmt);
		uint256 slashGGPAmt = calculateGGPSlashAmt(expectedAVAXRewardsAmt);
		setUint(keccak256(abi.encodePacked("minipool.item", index, ".ggpSlashAmt")), slashGGPAmt);


		emit GGPSlashed(nodeID, slashGGPAmt);


		Staking staking = Staking(getContractAddress("Staking"));
		staking.slashGGP(owner, slashGGPAmt);
	}

However, notice slashGGP, it decreaseGGPStake directly from stakerAddr.

	function slashGGP(address stakerAddr, uint256 ggpAmt) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) {
		Vault vault = Vault(getContractAddress("Vault"));
		decreaseGGPStake(stakerAddr, ggpAmt);
		vault.transferToken("ProtocolDAO", ggp, ggpAmt);
	}

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

	function subUint(bytes32 key, uint256 amount) internal {
		gogoStorage.subUint(key, amount);
	}

	function subUint(bytes32 key, uint256 amount) external onlyRegisteredNetworkContract {
		uintStorage[key] = uintStorage[key] - amount;
	}

This will result in a revert if stakedGGP for staker is less than expectedAVAXRewardsAmt due to underflow for the staker.item storage. Below are conditions that might result in a revert.

1. drop in GGP price
2. rise in AVAX price
3. 365 days duration

This proof of concept in MinipoolManager.t.sol shows a 12 month validation period which the protocol incentivizes node operator to sign up for according to the docs. By just dropping the price of ggp in avax from 1 ether to 0.9 ether, the staked GGP is insufficient in covering the expectedAVAXRewardsAmt and therefore causes dos to recordStakeEnd with slash reverting.

@@ -434,11 +434,11 @@ contract MinipoolManagerTest is BaseTest {
        }

        function testRecordStakingEndWithSlash() public {
-               uint256 duration = 2 weeks;
+               uint256 duration = 365 days;
                uint256 depositAmt = 1000 ether;
                uint256 avaxAssignmentRequest = 1000 ether;
                uint256 validationAmt = depositAmt + avaxAssignmentRequest;
-               uint128 ggpStakeAmt = 200 ether;
+               uint128 ggpStakeAmt = 100 ether;

                vm.startPrank(nodeOp);
                ggp.approve(address(staking), MAX_AMT);
@@ -481,6 +481,7 @@ contract MinipoolManagerTest is BaseTest {
                vm.expectRevert(MinipoolManager.InvalidAmount.selector);
                minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 9 ether);

+               oracle.setGGPPriceInAVAX(0.9 ether, block.timestamp);
                minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);

                assertEq(vault.balanceOf("MinipoolManager"), depositAmt);

Tools Used

Foundry

Recommended Mitigation Steps

Recommend making ggp staked liquidable so that it can be liquidated in the event ggp staked dropped below expectedAVAXRewardsAmt and also making ggp staked amount required based on the duration of days staked.

Lines of code

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

Vulnerability details

Impact

According to the logic when syncRewards is called to distribute rewards, the nextRewardsCycleEnd has to be divisible by rewardsCycleLength (14 days). Therefore, if syncRewards is called when 13 days has passed after the last rewardsCycle has ended, the nextRewardsCycle length will only be 1 day. Rewards will be distributed unfairly in this case as the rewards amount will only need 1 day to be distributed.

Proof of Concept

In syncRewards, nextRewardsCycleEnd is calculated by adding rewardsCycleLength (14 days) to current block timestamp and dividing down by rewardsCycleLength to ensure that nextRewardsCycleEnd is divisible by rewardsCycleLength.

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

Therefore if 13 days have passed since the last rewards cycle has ended before syncRewards is called, the next rewardsCycle length will only be 1 day. With only 1 day of rewardsCycle length, the rewards will be distributed unfairly compared to other rewards cycle.

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

Tools Used

Manual Review

Recommended Mitigation Steps

Recommend adding rewardsCycleLength to current block timestamp and using it as nextRewardsCycleEnd.

uint32 nextRewardsCycleEnd = timestamp + rewardsCycleLength;