GoGoPool contest - unforgiven's results

Lines of code

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

Vulnerability details

Impact

node operators ggp rewards are distributed by function calculateAndDistributeRewards() which is called by Multisig and function calculateAndDistributeRewards() can only distribute current cycle rewards. the rewards are calculated based on user's avaxAssignedHighWater and code would reset the value of the avaxAssignedHighWater so it can be calculated from zero for the next cycle. but if for one cycle Multisig don't call this function for one of the node runners the value of the avaxAssignedHighWater won't get reset and its value would be wrong for the next cycle and in the next cycle user would receive wrong amount of rewards (more than what supposed to receive). the problem is that contract should keep track of avaxAssignedHighWater for users in each cycle separately and don't rely on external call to reset the value of the avaxAssignedHighWater for users.

Proof of Concept

This is calculateAndDistributeRewards() code:

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

As you can see the reward is calculated based on avaxAssignedHighWater and then code reset the value of the avaxAssignedHighWater by calling staking.resetAVAXAssignedHighWater(stakerAddr). so if in one cycle Multisig don't call this function in time for a user then the value of the avaxAssignedHighWater won't get reset for the user and protocol would use its value for the next cycle. this can cause wrong reward distribution because in the next cycle that user can have different Minipool and different staking amount and the issue can cause user to receive more rewards (other users lose their rewards). the bug would happen if multisig don't call this function in time and it doesn't require bad action from Multisig or wrong parameters, just by not calling this function by any reason the bug can happen.

Tools Used

VIM

Recommended Mitigation Steps

calculate avaxAssignedHighWater for users in each cycle separately (use two dimensions map that includes cycle number and user address) and don't rely on external actor to call and reset the value of the avaxAssignedHighWater for user.

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

there is no check that duration of the Minipool is less than 365 days and if user by mistake set very high value for duration and fails to run node properly user would lose very large number of his GGP collaterals because the dominator is 365 days when calculating slash amount and this would cause node runner to loss more funds.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L149-L175 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L191-L269 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L285-L303

Vulnerability details

Impact

When Minipool is in Withdrawable or Error state owner can call withdrawMinipoolFunds() and withdraw his funds and Minipool would transfer to Finished state. but when Minipool is in Withdrawable or Error state it's possible to call createMinipool() by anyone (function requireValidStateTransition() allows transaction from Withdrawable and Error state to Prelaunch state) and set Minipool status to Prelaunch and reset Minipool information. This would cause real Minipool owner to lose access to his funds and those funds would stuck in the contract address forever. attacker can perform this attack for any Minipool in those states and make users lose funds. as Minipools are created for 14day cycle and then multisig would recreate them so each 14 days Minipools would be in those states and attacker can perform the attack for every minipools (if multisig performing end of the stakcing and recreating minipool without delay then attacker can front-run recreateMinipool() call from multisig too, but in general if a minipool stays in Withdrawable or Error state then attacker can make minipool owner to lose his funds). the bug is critical because attacker can make all node runners lose their funds and totally break the protocol.

Proof of Concept

This is requireValidStateTransition() code:

	function requireValidStateTransition(int256 minipoolIndex, MinipoolStatus to) private view {
		bytes32 statusKey = keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status"));
		MinipoolStatus currentStatus = MinipoolStatus(getUint(statusKey));
		bool isValid;

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

As you can see code allows transferring state from Withdrawable and Error to Prelaunch state in this lines:

             else if (currentStatus == MinipoolStatus.Withdrawable || currentStatus == MinipoolStatus.Error) {
			isValid = (to == MinipoolStatus.Finished || to == MinipoolStatus.Prelaunch)

and This is createMinipool() code:

	function createMinipool(
		address nodeID,
		uint256 duration,
		uint256 delegationFee,
		uint256 avaxAssignmentRequest
	) external payable whenNotPaused {
		if (nodeID == address(0)) {
			revert InvalidNodeID();
		}

		ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
		if (
			// Current rule is matched funds must be 1:1 nodeOp:LiqStaker
			msg.value != avaxAssignmentRequest ||
			avaxAssignmentRequest > dao.getMinipoolMaxAVAXAssignment() ||
			avaxAssignmentRequest < dao.getMinipoolMinAVAXAssignment()
		) {
			revert InvalidAVAXAssignmentRequest();
		}

		if (msg.value + avaxAssignmentRequest < dao.getMinipoolMinAVAXStakingAmt()) {
			revert InsufficientAVAXForMinipoolCreation();
		}

		Staking staking = Staking(getContractAddress("Staking"));
		staking.increaseMinipoolCount(msg.sender);
		staking.increaseAVAXStake(msg.sender, msg.value);
		staking.increaseAVAXAssigned(msg.sender, avaxAssignmentRequest);

		if (staking.getRewardsStartTime(msg.sender) == 0) {
			staking.setRewardsStartTime(msg.sender, block.timestamp);
		}

		uint256 ratio = staking.getCollateralizationRatio(msg.sender);
		if (ratio < dao.getMinCollateralizationRatio()) {
			revert InsufficientGGPCollateralization();
		}

		// Get a Rialto multisig to assign for this minipool
		MultisigManager multisigManager = MultisigManager(getContractAddress("MultisigManager"));
		address multisig = multisigManager.requireNextActiveMultisig();

		// Create or update a minipool record for nodeID
		// If nodeID exists, only allow overwriting if node is finished or canceled
		// 		(completed its validation period and all rewards paid and processing is complete)
		int256 minipoolIndex = getIndexOf(nodeID);
		if (minipoolIndex != -1) {
			requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
			resetMinipoolData(minipoolIndex);
			// Also reset initialStartTime as we are starting a whole new validation
			setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".initialStartTime")), 0);
		} else {
			minipoolIndex = int256(getUint(keccak256("minipool.count")));
			// The minipoolIndex is stored 1 greater than actual value. The 1 is subtracted in getIndexOf()
			setUint(keccak256(abi.encodePacked("minipool.index", nodeID)), uint256(minipoolIndex + 1));
			setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".nodeID")), nodeID);
			addUint(keccak256("minipool.count"), 1);
		}

		// Save the attrs individually in the k/v store
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Prelaunch));
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".duration")), duration);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".delegationFee")), delegationFee);
		setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner")), msg.sender);
		setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".multisigAddr")), multisig);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpInitialAmt")), msg.value);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")), msg.value);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")), avaxAssignmentRequest);

		emit MinipoolStatusChanged(nodeID, MinipoolStatus.Prelaunch);

		Vault vault = Vault(getContractAddress("Vault"));
		vault.depositAVAX{value: msg.value}();
	}

As you can see code when Minipool's index exists code only checks that if Minipool state can transfer to Prelaunch (line requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch)) and there is no owner checking and also code reset Minipools information (call to resetMinipoolData() and parameter setting in lines 255 to 263) and msg.sender become new owner of the minipool. So if a Minipool is in Withdrawable or Error states, attacker can call createMinipool() for that Minipool and make the real owner of the Minipool lose his access to his funds. these are the steps attacker need to do:

1. user1 wants to create a Minipool for 14 days and calls createMinipool(nodeID1) and set 1000 AVAX for staking. code would create Minipool for nodeID1 and set user1 as owner.
2. multisig would call claimAndInitiateStaking() and recordStakingStart() to took Minipool funds and start staking AVAX for nodeID1.
3. after 14 days multisig would call recordStakingEnd() and would transfer node runner AVAX funds + rewards. now Minipool would be in Withdrawable state and owner can call withdrawMinipoolFunds() to withdraw his funds and rewards.
4. attacker who watches mempool would see multisig transaction and immediately after multisig transaction would call craeteMinipool(nodeID1) and because Minipool is in Withdrawable state and there is no owner check for already existing Minipools, so code would allow attacker to re launch the Minipool and code would reset Minipool information and set attacker as owner.
5. user1 would lose access to his funds and can never withdraw his funds from Minipool.

This attack is possible if multisig calls recordStakingError() and make Minipool to go to Error state. and attack is also possible if node runner set higher value for duration of the minipool and multisig would try to call recreateMinipool() after each 14 day period. in both this situations because Minipool is in Error or Withdrawable state attacker can call createMinipool() before multisig or Minipool owner and perform the attack.

Tools Used

VIM

Recommended Mitigation Steps

check the owner of the Minipool in createMinipool() when Minipool existed before or change the states of Minipools and add new states so the confilict between recreateMinipool() and createMinipool() won't happens.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L42-L54 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L132-L134 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L120-L124 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L113-L130 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L88-L109

Vulnerability details

Impact

by performing this attack, attacker can cause other users to lose their funds when they are depositing their AVAX tokens into TokenggAVAX pool. when ever users deposits AVAX token into TokenggAVAX, contract calculates shares amount based on percentage of the AVAX received amount to total AVAX balance of the contract. attacker can make the ratio AVAX to share very high (like 100 * 1e18) by early direct AVAX transfer to contract address and this would cause very high division error in convertToShares() function and users would lose their funds up to ratio when depositing or withdrawing their funds.

Proof of Concept

This is convertToShares() code which is used to calculate shares amount during the user's deposits:

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

As you can see if the totalAssets() where so much bigger than supply then the division error would be very high for shares amount calculation (up to totalAssets() / supply) and user would receive shares that their values are much less than AVAX tokens he deposited. attacker can cause totalAsset() / supply ratio to be very high (up to 1000 * 1e18) and perform this attack. again when withdrawing contract uses function previewWithdraw():

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

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

and if attacker performs the attack and create big totalAssets() / supply ratio then user's more shares would get burned each time he withdraws because of the rounding up and division error. To make a big totalAssets() / supply ratio attacker needs to transfer AVAX tokens (for example 1000 * 1e18) directly to the contract address when contract deployed recently and has no or low deposits, and call the syncRewards() function so those transferred tokens get considered as rewards (when block.timestamp is close to nextRewardsCycleEnd). then attacker needs to call deposit() and deposit 1 wei AVAX and mint 1 share. so total supply would be 1 and totalAssets() would be 1000 * 1e18 (after reward duration ends) and totalAssets() / supply = 1000 * 1e18. these are the steps attacker need to perform:

1. wait for TokenggAVAX to get deployed.
2. wait for block.timestamp to be near rewardsCycleLength multiple. (for example there is 1000 seconds for next cycle begin time).
3. in on transaction send 1000 * 1e18 AVAX tokens to the contract and call syncRewards() and contract would consider the 1000 * 1e18 amount as rewards and start increasing totalAssets() linearly up to 1000 * 1e18 until end of the cycle(in 1000 seconds).
4. and also deposit 1 wei AVAX into the contract and receive 1 share.
5. 1 seconds after this deposit, the totalAssets() would be about 1e18 (1000 seconds until end of the cycle and in each seconds contract would release 1e18 tokens) and supply would be 1 and every user depositing or withdrawing would lose up to 1e18 tokens because of the rounding error in division. in this second if users deposits 15 * 1e17 AVAX tokens then contract would mint 1 share for him which worth 1e18 AVAX and users loses 5 * 1e17 of his token in his deposits. and if user withdraws 1 wei AVAX tokens contract would burn his 1 shares and users would lose the rest of his funds,.
6. at end of the cycle the ratio of the totalAssets() / supply would be 1000 * 1e18 and users would lose up to 1000 * 1e18.

the success and impact of this attack depends on closeness of block.timestamp to the rewardsCycleLength multiple (closer to the multiple the more fund loss) and amount of AVAX attacker transfers directly into the contract address. in general attacker can create high PPS ratio (the exact value of PPS that attacker can create may be differ based on the situation). the point is PPS would not get decreased after attack and users would lose funds afterwards and contract's broken state is not recoverable.

Tools Used

VIM

Recommended Mitigation Steps

check for minimum deposit amount or add extra decimals for share amount.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L187-L216

Vulnerability details

Impact

Functions registerContract(), unregisterContract() and upgradeExistingContract() are used for register and unregister and updating protocol contracts but they don't have any checks for function parameters and if Guardian call them by wrong values then the whole logics of the protocol can be broken and users lose funds. for example registerContract() don't check that provided name or address is not assigned before and if Guardian calls it with duplicate name or address then one address would have two names or two address would have same names and because Vault stores balance for contract names so two address can access each others balance by mistake and funds can be lost. also function upgradeExistingContract() would try to register new address and then unregister the old address which would be wrong action because unregister the old address would remove name to address mapping and protocol would be in wrong state if guardian calls upgradeExistingContract(). this is critical bug because it would happen always even if Guardian call the function with correct values.

Proof of Concept

This is registerContract(), unregisterContract() and upgradeExistingContract() codes:

	/// @notice Register a new contract with Storage
	/// @param addr Contract address to register
	/// @param name Contract name to register
	function registerContract(address addr, string memory name) public onlyGuardian {
		setBool(keccak256(abi.encodePacked("contract.exists", addr)), true);
		setAddress(keccak256(abi.encodePacked("contract.address", name)), addr);
		setString(keccak256(abi.encodePacked("contract.name", addr)), name);
	}

	/// @notice Unregister a contract with Storage
	/// @param addr Contract address to unregister
	function unregisterContract(address addr) public onlyGuardian {
		string memory name = getContractName(addr);
		deleteBool(keccak256(abi.encodePacked("contract.exists", addr)));
		deleteAddress(keccak256(abi.encodePacked("contract.address", name)));
		deleteString(keccak256(abi.encodePacked("contract.name", addr)));
	}

	/// @notice Upgrade a contract by unregistering the existing address, and registring a new address and name
	/// @param newAddr Address of the new contract
	/// @param newName Name of the new contract
	/// @param existingAddr Address of the existing contract to be deleted
	function upgradeExistingContract(
		address newAddr,
		string memory newName,
		address existingAddr
	) external onlyGuardian {
		registerContract(newAddr, newName);
		unregisterContract(existingAddr);
	}

As you can see code doesn't check the values provided by Guardian, this can serious problem if Guardian calls this functions out of order or by wrong values.

• function registerContract() doesn't check that provided name or address doesn't registered before and it may add name or address multiple times which would cause conflicts and collision.

• function unregisterContract() doesn't check that provided address is registered before.

• function upgradeExistingContract() doesn't check that newAddress and existingAddress are different and also it first resisters newAddress and then unregister the existingAddress, this would always create a broken state in protocol storage.

• scenario 1: Guardian adding two address with same name

1. Guardian would call registerContract(address1, "ClaimNodeOp") and code would set contract.exists.address1 = true, contract.address.ClaimNodeOp = address1 and contract.name.address1 = ClaimNodeOp.
2. then Guardian would call registerContract(address2, "ClaimNodeOp") and code would set contract.exists.address2 = true, contract.address.ClaimNodeOp = address2 and contract.name.address2 = ClaimNodeOp.
3. now both address1 and address2 has the same name and this can cause balance mismatch in the Vault. for example calls like this vault.withdrawToken() from address2 would use balance of address1 because they have same name.

• scenario 2: Guardian set 0 address for one of the names

1. Guardian would call registerContract(0x0 , "TokenggAVAX") and code would set address 0x0 for that name.
2. other contract would try to interact with address 0x0 and the calls won't fail and their logics works but TokenggAVAX logics won't get called which result in fund loss.
3. for example contract may send funds to 0x0 address or they assume they received funds by calling a function of 0x0 address.

• scenario 3: Guardian calls upgradeExistingContract() for already existing name.

1. address1, "ClaimNodeOp" has been added by Guardian.
2. Guardian wants to update "ClaimNodeOp" address and he calls upgradeExistingContract(address2, "ClaimNodeOp", address1).
3. code would fist tries to register address2 and would call registerContract(address2, "ClaimNodeOp") which would set contract.exists.address2 = true, contract.address.ClaimNodeOp = address2 and contract.name.address2 = ClaimNodeOp.
4. then code would unregister address1 by calling unregisterContract(address1) and it would remove contract.address.ClaimNodeOp = address2 from storage.
5. so the mapping of name to address would be removed and storage would be in the wrong state and protocol won't work.

Tools Used

VIM

Recommended Mitigation Steps

each functions should perform some basic proper checks like already registered, not registered, not zero address and name before performing the actions. and function upgradeExistingContract() should first unregister the old address then register the name address and name.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L285-L303 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L526-L533 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L149-L175

Vulnerability details

Impact

After node runner created a Minipool he can only withdraw his funds by calling cancelMinipool() when Minipool is in Prelaunch state or withdraw his funds by calling withdrawMinipoolFunds() when Minipool is in Withdrawable or Error state. so the only way for node runner to withdraw his funds after Minipool staking starts is call withdrawMinipoolFunds() when Minipool goes to Withdrawable or Error states. but when Minipool is in Withdrawable or Error states the Multisig can call finishFailedMinipoolByMultisig() and set the state of the Minipool to Finished and it would cause owner of the Minipool to lose access to his funds and those funds would stuck in the contract forever. function finishFailedMinipoolByMultisig() should have transfer Minipool owner funds before changing the state of the Minipool and in current implementation node runners would lose their funds whenever this functions get called by Multisig. even so this function is only callable by Multisig but any usage of this function would result in funds loss for bot state Withdrawable or Error so the logic of the function has flaw which cause fund loss and Multisig would make node runners lose their funds without even knowing it and the exploit does not require Multisig to be a bad actor and even if multisig do his normal action the node runners would lose their funds.

Proof of Concept

This is finishFailedMinipoolByMultisig() code:

	/// @notice Multisig can move a minipool from the error state to the finished state, after a human review of the error
	/// @param nodeID 20-byte Avalanche node ID
	function finishFailedMinipoolByMultisig(address nodeID) external {
		int256 minipoolIndex = onlyValidMultisig(nodeID);
		requireValidStateTransition(minipoolIndex, MinipoolStatus.Finished);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Finished));
		emit MinipoolStatusChanged(nodeID, MinipoolStatus.Finished);
	}

As you can see there is no code for transferring Minipool owner funds and code just changes status of the Minipool to Finished and this function is callable by Multisig whenever Minipool is in Withdrawable or Error state because requireValidStateTransition(minipoolIndex, MinipoolStatus.Finished) allows transferring to Finished state from Withdrawable and Error state (line 163 and 164 of MinipoolManager):

		} else if (currentStatus == MinipoolStatus.Withdrawable || currentStatus == MinipoolStatus.Error) {
			isValid = (to == MinipoolStatus.Finished || to == MinipoolStatus.Prelaunch);

when Minipool is in Launch state, Minipool owner can only withdraw his staked funds for that Minipool by calling withdrawMinipoolFunds() and he needs to wait for Minipool to be in Withdrawable or Error state to be able to call withdrawMinipoolFunds() and if Minipool was transfer to another sate like Finished by others, then owner can never withdraw his funds for that Minipool.

according to the comment in the code: "Multisig can move a minipool from the error state to the finished state, after a human review of the error", so calling this function whenever Minipool has encountered error is normal routine by Multisig. but because there is no logic to transfer Minipool's owner funds in the finishFailedMinipoolByMultisig() so whenever this functions is get called the Minipool would be in Finished state and owner could never access and withdraw his AVAX funds for that Minipool and his funds would be lost. This is a critical issue because normal usage of function by Multisig (when Minipool is in Error state) would always result in fund lose for Minipool owner and issue this would happen for all the calls to this function.

Also another bug is that this function is callable when Minipool is in Withdrawable state and if Multisig calls this function by mistake or bad intention for those Minipools then owner of the Minipool would lose his funds.

Tools Used

VIM

Recommended Mitigation Steps

only allow this function to be callable when Minipool is in Error state and also transfer owner funds before changing the sate of the Minipool.

Lines of code

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

Vulnerability details

Impact

the value of RewardsStartTime shows when node runner started the minipool and validation rewards are generated by node runner. it is used to see if node runners are eligible for ggp rewards or not and node runners should run their node for minimum amount of time during the rewarding cycle to be eligible for rewards. but right now node runner can create a mimipool and cancel it (after waiting time) and even so the minipool generated no rewards and cancelled the value of RewardsStartTime won't get reset for node runner and in the end of the cycle node runner would be eligible for rewards (node runner can create another minipool near the end of cycle). so this issue would cause wrong reward distribution between node runners and code doesn't correctly track RewardsStartTime for node runners and malicious node runners can use this issue and receive rewards without running validation nodes for the minimum amount of required time.

Proof of Concept

This is cancelMinipool() and _cancelMinipoolAndReturnFunds() code:

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

	function _cancelMinipoolAndReturnFunds(address nodeID, int256 index) private {
		requireValidStateTransition(index, MinipoolStatus.Canceled);
		setUint(keccak256(abi.encodePacked("minipool.item", index, ".status")), uint256(MinipoolStatus.Canceled));

		address owner = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".owner")));
		uint256 avaxNodeOpAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxNodeOpAmt")));
		uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt")));

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

		staking.decreaseMinipoolCount(owner);

		emit MinipoolStatusChanged(nodeID, MinipoolStatus.Canceled);

		Vault vault = Vault(getContractAddress("Vault"));
		vault.withdrawAVAX(avaxNodeOpAmt);
		owner.safeTransferETH(avaxNodeOpAmt);
	}

As you can see there is no check that user's minipool count is zero and if it is to reset the value of RewardsStartTime for user so if a user creates a minipool in the start of the cycle and then cancel it after 5 days and wait for end of the cycle and start another minipool and increase his staking AVAX he would be eligible for ggp rewards (ClaimNodeOp.isEligible() would return true for that user even so the user didn't run node for the required amount of time in the cycle). these are the steps to exploit this:

1. node runner would create a minipool near start time of the ggp rewarding cycle and the value of RewardsStartTime would set for node runner.
2. after 5 days that node runner's minipool has not been launched by multisig (for any reason) node runner would call cancelMinipool() and code would cancel his minipool but won't reset RewardsStartTime for node runner.
3. after 20 days and near end of the gpp reward cycle node runner would create another minipool and start running node.
4. in the end even so node runner only start running node and earning reward near end of the reward cycle but code would count node runner as eligible for rewards because RewardsStartTime for node runner shows wrong value.

This bug would cause rewards to be distributed wrongly between node runners and malicious node runners can bypass required time for running nodes during reward cycle to be eligible for rewards.

Tools Used

VIM

Recommended Mitigation Steps

set the value of RewardsStartTime based on successfully finished minipools or when minipool is launched and user can't cancel minipool.

Lines of code

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

Vulnerability details

Impact

According to the doc "A minipool can only be canceled by the owner after a 5 day waiting period. This is to prevent griefing." (https://docs.gogopool.com/design/minipooldesign#canceled) but current implementation in the code would allows for node operators to create and cancel Minipools immediately. by exploiting this issue attackers can perform various malicious actions, the first is griefing, attacker can lock liquidity stakers AVAX and prevent other Minipools from using those AVAX tokens and when multising wants to create a minipool attacker can front-run and cancel the Minipool. the second scenario is that attacker can create minipool and set the value of rewardsStartTime for his address and then cancel the minipool and use that AVAX to create minipool for his other addresses set rewardsStartTime for those too. (setting the value of rewardsStartTime for multiple addresses with same AVAX by creating and cancelling Minipools all in one transaction). the other scenario is that attacker can prevent other node runners from creating Minipools by performing sandwich attack (create a minipool with that nodeId before node runner transaction and then node runner transaction would fail and then cancel the minipool). so this issue can have multiple impacts and attacker can broke basic functionality of the protocol which is creating minipool and using liqudity stakers funds and earning rewards.

Proof of Concept

This is cancelMinipool() code:

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

As you can see to check that wait period has been passed code uses block.timestamp - staking.getRewardsStartTime(msg.sender) < dao.getMinipoolCancelMoratoriumSeconds() and code assumes staking.getRewardsStartTime(msg.sender) is the creation time of the Minipool but that is not the creation time of this Minipool and can be creation time of user's first Minipool. so This bug can make attacker to create a Minipool and set the value of the staking.getRewardsStartTime(msg.sender) for his address and then after 5 days attacker can create and cancel minipools instantly and by this attacker can cause all sort of greifing for liquidity stakers and node runners and can break fundemental functionality of the protocol. scenario 1: attacker can prevent others from creating minipools by performing sandwich attack

1. attacker would wait for node runners createMinipool() transaction to get added to mempools.
2. attacker would create a create minipool transaction with higher gas price and with the same nodeId as user transaction and with 1000 AVAX.
3. attacker transaction would execute first and change the state of that nodeId and then user transaction would fail because that nodeId already in the prelunch state.
4. then attacker can cancel that minipool with another transaction (can be done in the same transaction).

by performing this attacker can prevent any minipool creation (by cooperating with mining pools attacker can make all the createMinipool() transactions to fail just by setting order of the transaction and only with 1000 AVAX and 10% ggp collateral). this would cause main functionality of the protocol to fail and node runners can't create minipools and liquidity stakers funds can't be used in node validations and no rewards would be generates for them.

attacker can perform other attacks too and exploit the instant minipool creation and cancellation without any penalty. for example by creating and cancelling minipool for his multiple address can set rewardsStartTime for his addresses while circulating same AVAX fund and in the end all his addresses would have rewardsStartTime and would be eligible for rewards while none of them has active minipools.

Tools Used

VIM

Recommended Mitigation Steps

store start time of each miniPool and use it for calculating passed time.

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

there is no check that duration of the Minipool is bigger than 14 days and a malicious node runner can set duration as 0 day and if he fails the calculated slash amount would be 0.

Lines of code

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

Vulnerability details

Impact

Function syncRewards() distributes rewards to TokenggAVAX holders, it linearly distribute cycle's rewards from block.timestamp to the cycle end time which is next multiple of the rewardsCycleLength (the end time of the cycle is defined and the real duration of the cycle changes). when a cycle ends syncRewards() should be called so the next cycle starts but if syncRewards() doesn't get called fast, then users depositing or withdrawing funds before call to syncRewards() would lose their rewards and those rewards would go to users who deposited funds after syncRewards() call. contract should try to start the next cycle whenever deposit or withdraw happens to make sure rewards are distributed fairly between users.

Proof of Concept

This is syncRewards() code:

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

As you can see whenever this function is called it starts the new cycle and sets the end of the cycle to the next multiple of the rewardsCycleLength and it release the rewards linearly between current timestamp and cycle end time. so if syncRewards() get called near to multiple of the rewardsCycleLength then rewards would be distributed with higher speed in less time. the problem is that users depositing funds before call syncRewards() won't receive new cycles rewards and early depositing won't get considered in reward distribution if deposits happen before syncRewards() call and if a user withdraws his funds before the syncRewards() call then he receive no rewards. imagine this scenario:

1. rewardsCycleLength is 10 days and the rewards for the next cycle is 100 AVAX.
2. the last cycle has been ended and user1 has 10000 AVAX deposited and has 50% of the pool shares.
3. syncRewards() don't get called for 8 days.
4. users1 withdraws his funds receive 10000 AVAX even so he deposits for 8 days in the current cycle.
5. users2 deposit 1000 AVAX and get 10% of pool shares and the user2 would call syncRewards() and contract would start distributing 100 avax as reward.
6. after 2 days cycle would finish and user2 would receive 100 * 10% = 10 AVAX as rewards for his 1000 AVAX deposit for 2 days but user1 had 10000 AVAX for 8 days and would receive 0 rewards.

so rewards won't distribute fairly between depositors across the time and any user interacting with contract before the syncRewards() call can lose his rewards. contract won't consider deposit amounts and duration before syncRewards() call and it won't make sure that syncRewards() logic would be executed as early as possible with deposit or withdraw calls when a cycle ends.

Tools Used

VIM

Recommended Mitigation Steps

one way to solve this is to call syncRewards() logic in each deposit or withdraw and make sure that cycles start as early as possible (the revert "SyncError()" in the syncRewards() should be removed for this).

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L215-L223 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L205-L213

Vulnerability details

Impact

Functions maxWithdraw() and maxRedeem() returns max amount of assets or shares owner would be able to withdraw taking into account liquidity in the TokenggAVAX contract, but logics don't consider that when user withdraws the withdrawal amounts subtracted from totalReleasedAssets (in beforeWithdraw() function) so the maximum amounts that can user withdraws should always be lower than totalReleasedAssets (which shows all the deposits and withdraws) but because functions maxWithdraw() and maxRedeem() uses totalAssets() to calculate available AVAX which includes deposits and current cycle rewards so those functions would return wrong value (whenever the return value is bigger than totalReleaseAssets then it would be wrong)

Proof of Concept

This is beforeWithdraw() code:

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

This is beforeWithdraw() code which is called whenever users withdraws their funds and as you can see the amount of withdrawal assets subtracted from totalReleaseAssets so withdrawal amounts can never be bigger than totalReleaseAssets. This is maxWithdraw() code:

	function maxWithdraw(address _owner) public view override returns (uint256) {
		if (getBool(keccak256(abi.encodePacked("contract.paused", "TokenggAVAX")))) {
			return 0;
		}
		uint256 assets = convertToAssets(balanceOf[_owner]);
		uint256 avail = totalAssets() - stakingTotalAssets;
		return assets > avail ? avail : assets;
	}

As you can see to calculate available AVAX in the contract address code uses totalAssets() - stakingTotalAssets and totalAssets() shows deposits + current cycle rewards so totalAssets() is bigger than totalReleaseAssets and the value of the totalAssets() - stakingTotalAssets can be bigger than totalReleaseAssets and if code returns avail as answer then the return value would be wrong. imagine this scenario:

1. totalReleaseAssets is 10000 AVAX.
2. stakingTotalAssets is 1000 AVAX.
3. current cycle rewards is 4000 AVAX and block.timestamp is currently in the middle of the cycle so current rewards is 2000 AVAX.
4. totalAssets() is totalReleaseAssets + current rewards = 10000 + 2000 = 12000.
5. contract balance is 10000 + 4000 - 1000 = 13000 AVAX.
6. user1 has 90% contract shares and calls maxWithdraw() and code would calculate user assets as 10800 AVAX and available AVAX in contract as totalAssets() - stakingTotalAssets = 12000 - 1000 = 11000 and code would return 10800 as answer.
7. now if user1 withdraws 10800 AVAX code would revert in the function beforeWithdraw() because code would try to execute totalReleaseAssets = totalReleaseAssets - amount = 10000 - 10800 and it would revert because of the underflow. so in reality user1 couldn't withdraw 10800 AVAX which was the return value of the maxWithdraw() for user1.

the root cause of the bug is that the withdrawal amount is subtracted from totalReleaseAssets and so max withdrawal can never be totalReleaseAssets and function maxWithdraw() should never return value bigger than totalReleaseAssets. (the bug in function maxRedeem() is similar)

This bug would cause other contract or front end calls to fail, for example if the logic is something like this:

   amount = maxWithdraw(user);
   TokenggAVAX.withdrawAVAX(amount);

according the function definitions this code should work bug because of the the issue there are situations that this would revert and other contracts and UI can't work properly with the protocol.

Tools Used

VIM

Recommended Mitigation Steps

consider totalReleaseAssets in max withdrawal amount too.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L241-L247 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L180-L189 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L142-L146 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L111-L130

Vulnerability details

Impact

The last users can't withdraw all of their funds during a cycle and there would be locked funds in the TokenggAVAX contract always. when users withdraw their funds contract subtract the withdrawal amount from totalReleasedAssets which only shows deposits and withdraws and user can withdraw their ongoing cycle rewards and because totalReleasedAssets doesn't include ongoing rewards so if the withdrawal amounts get higher then totalReleasedAssets would be 0 and the reset of users can't withdraw their funds in that cycle.

Proof of Concept

This is withdrawAVAX() code in TokenggAVAX contract:

	function withdrawAVAX(uint256 assets) public returns (uint256 shares) {
		shares = previewWithdraw(assets); // No need to check for rounding error, previewWithdraw rounds up.
		beforeWithdraw(assets, shares);
		_burn(msg.sender, shares);

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

		IWAVAX(address(asset)).withdraw(assets);
		msg.sender.safeTransferETH(assets);
	}

As you can see code calls beforeWithdraw() and sends the value of withdrawal assets and share amount. this is beforeWithdraw() code:

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

As you can see code subtracts withdrawal asset amount from totalReleasedAssets. the value of totalReleasedAssets shows total assets that are deposited to the contract and users didn't withdraw them. in the beginning of each reward cycle code updates the value of totalReleasedAssets and add rewards amount in the last cycle to it but during the cycle totalReleasedAssets doesn't include the current cycle rewards and only shows the deposits amount, but total assets that users can withdraw is deposits+rewards so if last users wants to withdraw their funds because withdraw amount would be bigger than totalReleasedAssets, so underflow would happen on beforeWithdraw() and users can't withdraw their assets. imagine this scenario:

1. user1 deposited 9000 AVAX into the pool contract and has 90 share in the pool
2. user2 deposited 1000 AVAX into the pool contract and has 10 share in the pool
3. totalReleasedAssets value is 10000. the last reward cycle rewards are 0 AVAX and in the last cycle 2000 AVAX has been generated and it is in contract balance to be distributed in the next reward cycle. contract AVAX balance is 12000 AVAX.
4. function syncRewards() gets called and the value of lastRewardsAmt set to 2000 and contract would start calculate rewards for users linearly during the cycle.
5. half of the rewards cycle has been passed and function totalAssets() would return 11000 AVAX (10000 deposits and 1000 cycle rewards).
6. there is no active minipool and all the AVAX balance of TokenggAVAX contract is in its own address.
7. user1 shares would be 90% of 11000 AVAX (totalAssets() in the middle of the cycle) which is 90 * 11000 / 100 = 9900 AVAX. users1 would call withdraw() and would withdraw all of his shares and contract would transfer 9900 AVAX to user1 and the new value of the totalReleasedAssets would be 10000 - 9900 = 100 AVAX.
8. now user2 owns all the shares in the contract but user2 can't withdraw his shares because if user2 tries to withdraw more than 100 AVAX because of the subtract in the beforeWithdraw() the underflow would happen and transaction would revert and userr2 rewards would stuck in the contract until next cycle and even in the next cycle there would be new rewards and the bug would continue happening in the next cycles for the last assets and there would always were some assets that sticked in the contract.

the root cause of the bug is that code tries to subtract withdrawal amounts from totalReleasedAssets but the value of totalReleasedAssets doesn't include the ongoing reward cycle and if users withdraws their funds (which is deposits+cycle rewards) then the value of totalReleaseAssets would be small enough that when other users wants to withdraw their funds transaction would revert because of the underflow.

Tools Used

VIM

Recommended Mitigation Steps

add a variable named rewardWithdrawalAmount and if totalReleaseAssets is 0 then add those withdrawal amounts to rewardWithdrawalAmount and keep track of rewards withdrawals during the reward cycle and in the next cycle update the value of the totalReleaseAssets based on new rewards and rewards withdrawals.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L155-L197 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L199-L233

Vulnerability details

Impact

Function startRewardsCycle() is Public function that will run a GGP rewards cycle if possible but if the number of the enabled Multisigs were 0 then calling this function would fail because of division by zero error. there may be some moments when number of enabled Multisigs are 0x0 for any reason and this issue would cause ggp rewarding feature of protocol to be broken in those situations so some basic functionalities would not work and users can't access their rewards,.

Proof of Concept

This is function distributeMultisigAllotment() code which is called by startRewardsCycle():

	function distributeMultisigAllotment(
		uint256 allotment,
		Vault vault,
		TokenGGP ggp
	) internal {
		MultisigManager mm = MultisigManager(getContractAddress("MultisigManager"));

		uint256 enabledCount;
		uint256 count = mm.getCount();
		address[] memory enabledMultisigs = new address[](count);

		// there should never be more than a few multisigs, so a loop should be fine here
		for (uint256 i = 0; i < count; i++) {
			(address addr, bool enabled) = mm.getMultisig(i);
			if (enabled) {
				enabledMultisigs[enabledCount] = addr;
				enabledCount++;
			}
		}

		// Dirty hack to cut unused elements off end of return value (from RP)
		// solhint-disable-next-line no-inline-assembly
		assembly {
			mstore(enabledMultisigs, enabledCount)
		}

		uint256 tokensPerMultisig = allotment / enabledCount;
		for (uint256 i = 0; i < enabledMultisigs.length; i++) {
			vault.withdrawToken(enabledMultisigs[i], ggp, tokensPerMultisig);
		}
	}

As you can see code tries to calculates each Multisig rewards in the line tokensPerMultisig = allotment / enabledCount but if enabled Multisig counts were zero then the code would revert and function startRewardsCycle() won't be callable and this would cause node runners and DAO to not receive their ggp rewards in time. blocking the rewards long enough (more than that cycle length) can cause total reward loss of that cycle and there is no enforce in the protocol to make sure there is always one Multisig enabled and for any reason there could be some times that all Multisigs are disabled.

Tools Used

VIM

Recommended Mitigation Steps

check for zero amount and if there is zero Multisig then handle Multisig rewards in another way(set rewards as zero).