GoGoPool contest - SmartSek's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L670-L683

Vulnerability details

Impact

If operator sets a long duration slashing will likely revert, liquid stakers won't be compensated and operator will get to keep all his GGP.

Proof of Concept

The fact ggp is inflationary significantly increases the likelihood the price will decrease over time.

If duration is long enough and the price of ggp drops than Operator will not have enough GGP to be slashed.

The code snippet below shows how the slash amount would be 200 ggp in a situation where operator only had 100 ggp to be slashed.

uint public slash;
    uint public expectedRewardAmount;
    function testPrice() external { // 1000 staked
        // GGP starting Price In Avax = 1 ether

        // getExpectedAVAXRewardsAmt(365 days, 1000)
        // staked = staked * 1e18;// 1000/10 = 100
        uint duration = 365 days;
        uint liqStakersAmt = 1000 ether;
        expectedRewardAmount = (liqStakersAmt.mulWadDown(0.1 ether) * duration)/ 365 days;
        // calculateGGPSlashAmt(expectedRewardAmount)
        uint ggpPriceInAvax = 0.5 ether;
        slash = expectedRewardAmount.divWadDown(ggpPriceInAvax);
    }

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L257-L258

		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".duration")), duration); 

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

	function calculateGGPSlashAmt(uint256 avaxRewardAmt) public view returns (uint256) { 
		Oracle oracle = Oracle(getContractAddress("Oracle"));
		(uint256 ggpPriceInAvax, ) = oracle.getGGPPriceInAVAX();
		return avaxRewardAmt.divWadDown(ggpPriceInAvax); 
	} 

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L670-L683

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

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L557-L561

	function getExpectedAVAXRewardsAmt(uint256 duration, uint256 avaxAmt) public view returns (uint256) {
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		uint256 rate = dao.getExpectedAVAXRewardsRate(); //@audit should be 0.1 (10% annually)
		return (avaxAmt.mulWadDown(rate) * duration) / 365 days;
	}

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

		decreaseGGPStake(stakerAddr, ggpAmt); 

Recommended Mitigation Steps

Use the validator duration of 14 days to slash ggp rewards.

Lines of code

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

Vulnerability details

Impact

An attacker/early user can deposit 1 wei in the vault and increase the price per share by sending a very high value of the underlying directly to the vault, causing next vault depositors to:

• not be able to deposit less than the very high share price set by the attacker.
• lose value due to rounding error.

Proof of Concept

• Malicious user Alice can deposit() with 1 wei of asset token to get 1 wei of shares.
• Next, Alice sends 10000e18 -1 of asset tokens and inflate the price per share from 1 to 1e22.
• Subsequent depositor who deposits shares, eg 19999e18 of assets, will only receive 1 wei of shares token.
• Victim will lose 9999e18 if they redeem() right after deposit() due to precision loss.

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

	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());
	}
	
	function convertToAssets(uint256 shares) public view virtual returns (uint256) { //@audit previewRedeem
		uint256 supply = totalSupply; // Saves an extra SLOAD if totalSupply is non-zero.

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

Recommended Mitigation Steps

Send first LP tokens to address(0) as Uniswap does.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L324-L343

Vulnerability details

Impact

Recreating canceled pool is allowed even though it uses other operator's money. If processed is accidentally repeated multiple times it can drain MinipoolManager.sol

Proof of Concept

When can pool be cancelled? - 5 day window after staking start time during Prelaunch state, set in create.

Pool is created

• increaseAVAXStake avaxStaked = msg.value = avaxNodeOpAmt
• increaseAVAXAssigned avaxAssigned = avaxAssignmentRequest = msg.value (should be 1:1 to msg.value)

Pool is canceled (within 5 days)

• decreaseAVAXStake avaxStaked = 0
• avaxNodeOpAmt still equal msg.value when created.
• decreaseAVAXAssigned even though "avaxLiquidStakerAmt" is not updated
• Staked AVAX is sent back to owner of minipool. This part is relevant because when pool is recreated AVAX is not returned.
• avaxBalances[minipoolmanager] = gets subtracted

Pool is recreated (Prelaunch)

• increaseAVAXStake avaxStaked = avaxNodeRewardOpAmt = 0 - avaxNodeRewardOpAmt = 0. avaxNodeRewardOpAmt is only set when calling RecordingStakingEnd
• avaxNodeOpAmt = previous msg.value when created. (even though the staking balance has been decreased)
  • avaxNodeOpRewardAmt = avaxNodeOpAmt + avaxNodeRewardOpAmt = msg.value + 0;
• increaseAVAXAssigned avaxAssigned = avaxNodeOpAmt -Does this pass the collateralization ratio? - yes the GGPstaked is not affected.

-- Extremely unlikely scenario: If steps above are repeated multiple times the MinipoolManager.sol balance will be drained. Continuing execution flow:

Rialto calls claimAndInitiateStaking

• ggAVAX.withdrawForStaking(avaxLiquidStakerAmt); still goes through. doesn't use funds it shouldnt
• vault.withdrawAVAX(avaxNodeOpAmt);
  • if theres no balance left: Revert occurs and pool cannot be recreated.
  • if there is sufficient left: deposits from other users will be used.
• totalAvaxAmt is staked on Avalanche even though the avaxNodeOpAmt half is not the operator's money.

recordStakingStart

• Gets executed normally.

recordStakingEnd

• Gets executed normally.

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L273-L283

	function cancelMinipool(address nodeID) external nonReentrant {
		Staking staking = Staking(getContractAddress("Staking"));
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		int256 index = requireValidMinipool(nodeID);
		onlyOwner(index);
		// make sure they meet the wait period requirement
		if (block.timestamp - staking.getRewardsStartTime(msg.sender) < dao.getMinipoolCancelMoratoriumSeconds()) { 
			revert CancellationTooEarly();
		}
		_cancelMinipoolAndReturnFunds(nodeID, index); 
	}

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L324-L343

	function claimAndInitiateStaking(address nodeID) external {
		int256 minipoolIndex = onlyValidMultisig(nodeID);
		requireValidStateTransition(minipoolIndex, MinipoolStatus.Launched);

		uint256 avaxNodeOpAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")));
		uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")));

		// Transfer funds to this contract and then send to multisig
		ggAVAX.withdrawForStaking(avaxLiquidStakerAmt);
		addUint(keccak256("MinipoolManager.TotalAVAXLiquidStakerAmt"), avaxLiquidStakerAmt);

		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Launched));
		emit MinipoolStatusChanged(nodeID, MinipoolStatus.Launched);

		Vault vault = Vault(getContractAddress("Vault"));
		vault.withdrawAVAX(avaxNodeOpAmt);

		uint256 totalAvaxAmt = avaxNodeOpAmt + avaxLiquidStakerAmt;
		msg.sender.safeTransferETH(totalAvaxAmt);
	}

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L444-L478

	function recreateMinipool(address nodeID) external whenNotPaused { 
		int256 minipoolIndex = onlyValidMultisig(nodeID);
		requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
		Minipool memory mp = getMinipool(minipoolIndex);
		// Compound the avax plus rewards
		// NOTE Assumes a 1:1 nodeOp:liqStaker funds ratio
		uint256 compoundedAvaxNodeOpAmt = mp.avaxNodeOpAmt + mp.avaxNodeOpRewardAmt;
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")), compoundedAvaxNodeOpAmt);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")), compoundedAvaxNodeOpAmt);

		Staking staking = Staking(getContractAddress("Staking"));
		// Only increase AVAX stake by rewards amount we are compounding
		// since AVAX stake is only decreased by withdrawMinipool() 
		staking.increaseAVAXStake(mp.owner, mp.avaxNodeOpRewardAmt); 
		staking.increaseAVAXAssigned(mp.owner, compoundedAvaxNodeOpAmt); 
		staking.increaseMinipoolCount(mp.owner); 

		if (staking.getRewardsStartTime(mp.owner) == 0) {/
			// Edge case where calculateAndDistributeRewards has reset their rewards time even though they are still cycling
			// So we re-set it here to their initial start time for this minipool
			staking.setRewardsStartTime(mp.owner, mp.initialStartTime);
		}

		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		uint256 ratio = staking.getCollateralizationRatio(mp.owner);
		if (ratio < dao.getMinCollateralizationRatio()) {
			revert InsufficientGGPCollateralization();
		}

		resetMinipoolData(minipoolIndex);

		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Prelaunch));

		emit MinipoolStatusChanged(nodeID, MinipoolStatus.Prelaunch);
	}

Recommended Mitigation Steps

Do not allow canceled Minipools to be recreated.

Lines of code

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

Vulnerability details

Impact

Users might not be able to withdraw funds that they are entitled to and are present in the contract.

Proof of Concept

depositFromStaking() deposits rewards in Staking.sol, however users might not be able withdraw or redeem the amount their entitled to because totalReleasedAssets has not been updated with the new asset.balanceOf(address(this)) after depositFromStaking() was called.
Users will need to wait until rewardsCycleEnd to call syncRewards to update totalReleasedAssets with the current asset.balanceOf(address(this)).

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

	function depositFromStaking(uint256 baseAmt, uint256 rewardAmt) public payable onlySpecificRegisteredContract("MinipoolManager", msg.sender) {
		uint256 totalAmt = msg.value;
		if (totalAmt != (baseAmt + rewardAmt) || baseAmt > stakingTotalAssets) {
			revert InvalidStakingDeposit();
		}

		emit DepositedFromStaking(msg.sender, baseAmt, rewardAmt);
		stakingTotalAssets -= baseAmt;
		IWAVAX(address(asset)).deposit{value: totalAmt}(); 
	} 

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

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

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

Recommended Mitigation Steps

Update totalReleasedAssets in depositFromStaking()

Note: No funds here are at permanent risk, however because liquidity for stakers is a crucial part of the protocol I believe this to be a high severity.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L56-L85

Vulnerability details

Impact and Walktrough

Scenario (Impact) 1: Operator will be eligible for rewards even though he didnt't earn any. This will lead Rialto not being able to distribute rewards to some operator's due a difference in actual rewards and rewards to be distributed.

• Operator calls createMinipool()
• Rialto calls recordStakingStart() (status: Staking) -> increaseAVAXAssignedHighWater is called
• Operator somehow immediately triggers Rialto to call recordStakingError()
• Operator calls withdrawMinipoolFunds and get his AVAX back (minipool.count is not decreased).
• By this point increaseAVAXAssignedHighWater has not been decreased yet.
• Once reward cycle is complete Rialto calls calculateAndDistributeRewards(). - Because minipool.count count has not been decreased and therefore, rewardsStartTime is still unchanged Operator is still eligible. - Operator gets GGP reward even though he could have received no AVAX rewards neither staked for any significant period of time to earn his rewards. - resetAVAXAssignedHighWater() is called so this will not repeat and next cycle GGP rewards will be zero if nothing else changes (thankfully).

Scenario (Impact) 2: isEligible() returns true when it should return false instead. Leading to Rialto wasting gas by calling calculateAndDistributeRewards() mulitple times to distribute 0 rewards for Operator that have not staked during the current reward cycle. Potentially even leading to a DOS depending on the Rialto function logic that calls calculateAndDistributeRewards() for every Operator.

• Operator calls createMinipool()
• Rialto calls claimAndInitiateStaking()
• Rialto calls recordStakingError()
• Operator calls withdrawMinipoolFunds() (optional)
• staking.decreaseMinipoolCount() is never called
• Rialto calls calculateAndDistributeRewards(). - Because minipool.count count has not been decreased and therefore, rewardsStartTime is still unchanged Operator is still eligible.

Proof of Concept

decreaseMiniPoolCount is only called in recordStakingEnd() and _cancelMinipoolAndReturnFunds() Pools can't be canceled unless if in Prelaunch status.

if (currentStatus == MinipoolStatus.Prelaunch) {
			isValid = (to == MinipoolStatus.Launched || to == MinipoolStatus.Canceled);
```
Consequently every pool that roughly follows the path of `Error`->`Finished` and does not reach the stage where Rialto calls `recordStakingEnd()` can not be canceled.
This means `minipool.count` will never be decreased even after `withdrawMinipoolFunds()` is called for `Error` status pools.
`minipool.count` will then never be equal zero even when pool is `Finished`.
Consequently `rewardsStartTime` will never be reset to zero when Rialto calls `calculateAndDistributeRewards`.
```
		uint256 minipoolCount = staking.getMinipoolCount(stakerAddr);
		if (minipoolCount == 0) {
			staking.setRewardsStartTime(stakerAddr, 0); 
		} 
```
Rialto will end up calling `calculateAndDistributeRewards()` mulitple times to distribute zero rewards to every Operator that has no real active pools, but rather in a permanent `Error` or `Finished` status. Potentially also leading to a DOS depending on the function logic that calls `calculateAndDistributeRewards()` for every Operator.
This might also lead to attempting the distribution of rewards for an amount greater than the amount of real rewards generated. Consequently some user will not be able to be compensated fairly.

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L152-L175

```solidity
	function requireValidStateTransition(int256 minipoolIndex, MinipoolStatus to) private view {
		bytes32 statusKey = keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status"));
		MinipoolStatus currentStatus = MinipoolStatus(getUint(statusKey));
		bool isValid;
		//@audit come back to this and check if every state transition is done properly!
		if (currentStatus == MinipoolStatus.Prelaunch) {
			isValid = (to == MinipoolStatus.Launched || to == MinipoolStatus.Canceled);
		} else if (currentStatus == MinipoolStatus.Launched) {
			isValid = (to == MinipoolStatus.Staking || to == MinipoolStatus.Error);
		} else if (currentStatus == MinipoolStatus.Staking) {
			isValid = (to == MinipoolStatus.Withdrawable || to == MinipoolStatus.Error);
		} else if (currentStatus == MinipoolStatus.Withdrawable || currentStatus == MinipoolStatus.Error) {
			isValid = (to == MinipoolStatus.Finished || to == MinipoolStatus.Prelaunch);
		} else if (currentStatus == MinipoolStatus.Finished || currentStatus == MinipoolStatus.Canceled) {
			// Once a node is finished/canceled, if they re-validate they go back to beginning state
			isValid = (to == MinipoolStatus.Prelaunch);
		} else {
			isValid = false;
		}

		if (!isValid) {
			revert InvalidStateTransition();
		}
	}
```

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L56-L85

```solidity
	function calculateAndDistributeRewards(address stakerAddr, uint256 totalEligibleGGPStaked) external onlyMultisig {
		Staking staking = Staking(getContractAddress("Staking"));
		staking.requireValidStaker(stakerAddr);

		RewardsPool rewardsPool = RewardsPool(getContractAddress("RewardsPool"));
		if (rewardsPool.getRewardsCycleCount() == 0) {
			revert RewardsCycleNotStarted();
		}
		
		if (staking.getLastRewardsCycleCompleted(stakerAddr) == rewardsPool.getRewardsCycleCount()) { 
			revert RewardsAlreadyDistributedToStaker(stakerAddr);
		}
		staking.setLastRewardsCycleCompleted(stakerAddr, rewardsPool.getRewardsCycleCount());
		uint256 ggpEffectiveStaked = staking.getEffectiveGGPStaked(stakerAddr);
		uint256 percentage = ggpEffectiveStaked.divWadDown(totalEligibleGGPStaked);
		uint256 rewardsCycleTotal = getRewardsCycleTotal();
		uint256 rewardsAmt = percentage.mulWadDown(rewardsCycleTotal);
		if (rewardsAmt > rewardsCycleTotal) {
			revert InvalidAmount();
		}

		staking.resetAVAXAssignedHighWater(stakerAddr);
		staking.increaseGGPRewards(stakerAddr, rewardsAmt);

		// check if their rewards time should be reset 
		uint256 minipoolCount = staking.getMinipoolCount(stakerAddr);
		if (minipoolCount == 0) {
			staking.setRewardsStartTime(stakerAddr, 0); 
		} 
	}
```

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L44-L52

```solidity
	/// @notice Determines if a staker is eligible for the upcoming rewards cycle
	/// @dev Eligiblity: time in protocol (secs) > RewardsEligibilityMinSeconds. Rialto will call this.
	function isEligible(address stakerAddr) external view returns (bool) {
		Staking staking = Staking(getContractAddress("Staking"));
		uint256 rewardsStartTime = staking.getRewardsStartTime(stakerAddr);
		uint256 elapsedSecs = (block.timestamp - rewardsStartTime);
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		return (rewardsStartTime != 0 && elapsedSecs >= dao.getRewardsEligibilityMinSeconds());
	}
```

## Recommended Mitigation Steps
Call `decreaseMinipoolCount()` whenever pool is likely to be created or recreated or it's current status can be updated to `Prelaunch`.
Alternatively for `Error` and `Finished` pools to be canceled via valid state transition.