GoGoPool contest - sces60107's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L196 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L243

Vulnerability details

Impact

Minipool owners can call withdrawMinipoolFunds() to withdraw funds, but the state Withdrawable also enables anyone to call createMinipool(). If an attacker calls createMinipool() before withdrawMinipoolFunds() called by the minipool owner, the owner will lose funds.

Proof of Concept

The graph shows requireValidStateTransition states.

Assume Alice is a minipool owner of minipoolIndex 1, and the minipool is in Withdrawable state. Alice is able to call withdrawMinipoolFunds() to claim all AVAX funds. But at this time, an attacker can call createMinipool() on minipoolIndex 1 to create a new minipool. L243 will check requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch) which means that it’s valid from Withdrawable state to Prelaunch state. Finally the attacker will be the minipool owner of minipoolIndex 1 and Alice will lose her funds.

Tools Used

Manual Review

Recommended Mitigation Steps

In createMinipool, check the owner should be the same if the nodeId exists (getIndexOf(nodeID) != -1).

Lines of code

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

Vulnerability details

Impact

Attackers can manipulate the price per share in TokenggAVAX to take an unfair share of future users because the protocol allows users to deposit a insignificant amount of tokens.

Proof of Concept

A malicious early user can deposit() with 1 wei of asset token and get 1 wei of shares. Then he/she can send 10000e18 - 1 of asset tokens and inflate the price per share from 1 to an extreme value of 1e22

(1 + 10000e18 - 1) / 1 = 1e22

A future user who deposits 19999e18 will only receive 1 wei of shares token.

19999e18 * 1 / 10000e18 = 1

he/she would lose 9999e18 if they redeem() right after the deposit().

Tools Used

Manual Review

Recommended Mitigation Steps

Require minimum amount of share in deposit function and mint function.

Lines of code

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

Vulnerability details

Impact

Ocyticus.pauseEverything() is called when the defender wants to pause the whole protocol. However, it doesn’t pause RewardPool. So GGP will keep inflating while the protocol is paused. This is unfair since no one can create mini pool to share the inflated GGP when the protocol is paused.

Proof of Concept

Ocyticus.pauseEverything() doesn’t pause RewardPool

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

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

But GGP will keep inflating while the protocol is paused.

Tools Used

Manual Review

Recommended Mitigation Steps

Ocyticus.pauseEverything() should also pause RewardPool

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L528

Vulnerability details

Impact

Multisig can call finishFailedMinipoolByMultisig() to change the state to Finished. But the function doesn't check if the owner has already withdrawn. The minipool owner will lose funds if multisig forcibly or unexpectedly calls the function.

Proof of Concept

The graph shows requireValidStateTransition states.

If an error occurs when the user stakes, multisig will call recordStakingError to set the state to Error, and the function sends funds to the vault so the minipool owner can call withdrawMinipoolFunds() to claim later. But the finishFailedMinipoolByMultisig() function is able to set the state to Finished. If multisig calls finishFailedMinipoolByMultisig() before the minipool owner calls withdrawMinipoolFunds(), the minipool owner will lose funds.

Tools Used

Manual Review

Recommended Mitigation Steps

finishFailedMinipoolByMultisig should check whether the owner has withdrawn the funds.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/RewardsPool.sol#L84 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/RewardsPool.sol#L98 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/RewardsPool.sol#L82 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/RewardsPool.sol#L66

Vulnerability details

Impact

In RewardPool.inflate(), it computes inflation based on total inflation intervals elapsed, then it starts a new inflate interval and set RewardsPool.InflationIntervalStartTime to block.timestamp (inflationIntervalElapsedSeconds = (block.timestamp - getInflationIntervalStartTime())).

It should set a start time based on the number of inflation intervals that have elapsed. Otherwise, it could result in a waste of inflation time and make the inflation interval length almost twice longer.

Proof of Concept

RewardPool.inflate() computes inflation based on total inflation intervals elapsed

	function getInflationAmt() public view returns (uint256 currentTotalSupply, uint256 newTotalSupply) {
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		uint256 inflationRate = dao.getInflationIntervalRate();
		uint256 inflationIntervalsElapsed = getInflationIntervalsElapsed();
		currentTotalSupply = dao.getTotalGGPCirculatingSupply();
		newTotalSupply = currentTotalSupply;

		// Compute inflation for total inflation intervals elapsed
		for (uint256 i = 0; i < inflationIntervalsElapsed; i++) {
			newTotalSupply = newTotalSupply.mulWadDown(inflationRate);
		}
		return (currentTotalSupply, newTotalSupply);
	}


	function inflate() internal {
		…
		(uint256 currentTotalSupply, uint256 newTotalSupply) = getInflationAmt();

		…
	}

RewardPool.inflate() set RewardsPool.InflationIntervalStartTime to block.timestamp. (inflationIntervalElapsedSeconds = (block.timestamp - getInflationIntervalStartTime()))

	function inflate() internal {
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		uint256 inflationIntervalElapsedSeconds = (block.timestamp - getInflationIntervalStartTime());
		…

		addUint(keccak256("RewardsPool.InflationIntervalStartTime"), inflationIntervalElapsedSeconds);
		setUint(keccak256("RewardsPool.RewardsCycleTotalAmt"), newTokens);
	}

According to ProtocolDao.sol. The length of an inflation interval is 1 day by default.

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ProtocolDAO.sol#L40

		// GGP Inflation
		setUint(keccak256("ProtocolDAO.InflationIntervalSeconds"), 1 days);

Thus, if the elapsed time is 47 hours (1 day + 23 hours) and RewardPool.inflate() is called, only one interval is counted.

Tools Used

Manual Review

Recommended Mitigation Steps

set a proper start time of the new inflation interval.

	function inflate() internal {
		…

		addUint(keccak256("RewardsPool.InflationIntervalStartTime"), getInflationIntervalsElapsed() * dao.getInflationIntervalSeconds());
		setUint(keccak256("RewardsPool.RewardsCycleTotalAmt"), newTokens);
	}

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L484 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L221 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L459 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L437 https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/MinipoolManager.sol#L658

Vulnerability details

Impact

A node operator’s minipoolCount increases when calling MinipoolManager.createMinipool and MinipoolManager.recreateMinipool. And the count decreases only when calling MinipoolManager.cancelMinipool, MinipoolManager.recordStakingEnd and MinipoolManager.cancelMinipoolByMultisig.

But if a staking error occurred while registering the node as a validator. MinipoolManager.recordStakingError will be called. And the state transits to MinipoolStatus.Error. It doesn’t decrease the minipool count of the node operator. And MinipoolManager.cancelMinipool, MinipoolManager.recordStakingEnd and MinipoolManager.cancelMinipoolByMultisig cannot be called when the state is MinipoolStatus.Error.

Currently, if minipoolCount goes wrong, only ClaimNodeOp.calculateAndDistributeRewards will be affected. A node operator’s rewards time will never be reset. But it could lead to a potential disaster if minipoolCount is used in the future.

Proof of Concept

A node operator’s minipoolCount increases when calling MinipoolManager.createMinipool and MinipoolManager.recreateMinipool. Staking.increaseMinipoolCount is called

	function createMinipool(
		address nodeID,
		uint256 duration,
		uint256 delegationFee,
		uint256 avaxAssignmentRequest
	) external payable whenNotPaused {
		…

		Staking staking = Staking(getContractAddress("Staking"));
		staking.increaseMinipoolCount(msg.sender);
		…
	}

	function recreateMinipool(address nodeID) external whenNotPaused {
		…

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

		…
	}

And the count decreases only when calling MinipoolManager.cancelMinipool, MinipoolManager.recordStakingEnd and MinipoolManager.cancelMinipoolByMultisig. Staking.decreaseMinipoolCount is called.

	function cancelMinipool(address nodeID) external nonReentrant {
		…
		_cancelMinipoolAndReturnFunds(nodeID, index);
	}

	function cancelMinipoolByMultisig(address nodeID, bytes32 errorCode) external {
		…
		_cancelMinipoolAndReturnFunds(nodeID, minipoolIndex);
	}

	function _cancelMinipoolAndReturnFunds(address nodeID, int256 index) private {
		…

		Staking staking = Staking(getContractAddress("Staking"));
		staking.decreaseAVAXStake(owner, avaxNodeOpAmt);
		staking.decreaseAVAXAssigned(owner, avaxLiquidStakerAmt);

		staking.decreaseMinipoolCount(owner);

		…
	}

	function recordStakingEnd(
		address nodeID,
		uint256 endTime,
		uint256 avaxTotalRewardAmt
	) external payable {
		…

		Staking staking = Staking(getContractAddress("Staking"));
		staking.decreaseAVAXAssigned(owner, avaxLiquidStakerAmt);
		staking.decreaseMinipoolCount(owner);

		emit MinipoolStatusChanged(nodeID, MinipoolStatus.Withdrawable);
	}

But If MinipoolManager.recordStakingError is called. Staking.decreaseMinipoolCount won’t be called.

Tools Used

Manual Review

Recommended Mitigation Steps

minipooCount should correctly decrease when the state transits to MinipoolStatus.Error

Call staking.decreaseMinipoolCount(owner); in MinipoolManager.recordStakingError or MinipoolManager.finishFailedMinipoolByMultisig.

Summary

We list 5 low-critical findings and 1 non-critical finding:

• (low) block.timestamp is uint32 in TokenggAVAX
• (low) ProtocolDao.setClaimingContractPct should has reasonable check
• (low) ProtocolDAO.sol code for testing
• (low) open TODO
• (low) ERC4626Upgradeable.sol, ERC20Upgradeable.sol and BaseAbstract.sol should have storage gap
• (Non) RewardPool.startRewardsCycle doesn’t set a proper start time of the new reward cycle.

(low) block.timestamp is uint32 in TokenggAVAX

Impact

Solidity docs defined that block.timestamp is uint256, but in TokenggAVAX, it uses uint32 to store timestamps. The uint32.max is 4294967295 which is Sun Feb 07 2106, it means that the protocol will break when the maturity is greater than 2106.

https://ethereum.stackexchange.com/questions/28969/what-uint-type-should-be-declared-for-unix-timestamps

Proof of Concept

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

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

Tool used

Manual Review

Recommended Mitigation Steps

Define as uint256 or larger bits than 32.

(low) ProtocolDao.setClaimingContractPct should has reasonable check

Impact

ProtocolDao.setClaimingContractPct is used to set the percentage a contract is owed for a rewards cycle. But it doesn’t check that the total sum of percentage is 100%.

Proof of Concept

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ProtocolDAO.sol#L107

	/// @notice Set the percentage a contract is owed for a rewards cycle
	function setClaimingContractPct(string memory claimingContract, uint256 decimal) public onlyGuardian valueNotGreaterThanOne(decimal) {
		setUint(keccak256(abi.encodePacked("ProtocolDAO.ClaimingContractPct.", claimingContract)), decimal);
	}

Tool used

Manual Review

Recommended Mitigation Steps

setClaimingContractPct should set the percentages of all the contracts owed for a rewards cycle. Then it can check whether the sum of percentages is 100%.

(low) ProtocolDAO.sol code for testing

Impact

Test code should be removed in the production environment.

Proof of Concept

	/// @notice Set the rewards rate for validating Avalanche's p-chain
	/// @dev Used for testing
	function setExpectedAVAXRewardsRate(uint256 rate) public onlyMultisig valueNotGreaterThanOne(rate) {
		setUint(keccak256("ProtocolDAO.ExpectedAVAXRewardsRate"), rate);
	}

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ProtocolDAO.sol#L154-L158

Tools Used

Manual Review

Recommended Mitigation Steps

Remove these code.

(low) open TODO

Impact

These open TODO should be implemented.

Proof of Concept

BaseAbstract.sol
6:// TODO remove this when dev is complete

Staking.sol
203:    // TODO cant use onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) since we also call from increaseMinipoolCount. Wat do?

MinipoolManager.sol
412:            // TODO Revisit this logic if we ever allow unequal matched funds

Tools Used

Manual Review

Recommended Mitigation Steps

Implement these open TODO.

(low) ERC4626Upgradeable.sol, ERC20Upgradeable.sol and BaseAbstract.sol should have storage gap

Impact

The inherited upgradable contract should have storage gaps.

Proof of Concept

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

Tools Used

Manual Review

Recommended Mitigation Steps

Using storage gap:

    uint256[50] __gap;

https://docs.openzeppelin.com/contracts/3.x/upgradeable#storage_gaps

(Non) RewardPool.startRewardsCycle could set a proper start time of the new reward cycle.

Impact

In RewardPool.startRewardsCycle(), it would start a new reward cycle and set RewardsPool.RewardsCycleStartTime to block.timestamp. It could set a start time based on the number of reward cycles that have elapsed.

Proof of Concept

RewardPool.startRewardsCycle() set RewardsPool.RewardsCycleStartTime to block.timestamp. https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/RewardsPool.sol#L164

	function startRewardsCycle() external {
		…

		// Set start of new rewards cycle
		setUint(keccak256("RewardsPool.RewardsCycleStartTime"), block.timestamp);
		…
	}

According to ProtocolDao.sol. The length of a reward cycle is 28 days by default.

https://github.com/code-423n4/2022-12-gogopool/blob/main/contracts/contract/ProtocolDAO.sol#L33

		// RewardsPool
		setUint(keccak256("ProtocolDAO.RewardsCycleSeconds"), 28 days); // The time in which a claim period will span in seconds - 28 days by default

Thus, if the elapsed time is 55 days and you call RewardPool.startRewardsCycle(), only one reward cycle is counted.

Tools Used

Manual Review

Recommended Mitigation Steps

set a proper start time of the new reward cycle.

	function startRewardsCycle() external {
		…

		// Set start of new rewards cycle
		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		setUint(keccak256("RewardsPool.RewardsCycleStartTime"), getRewardsCycleStartTime() + getRewardsCyclesElapsed() * dao.getRewardsCycleSeconds());
		…
	}