GoGoPool contest - cccz's results

Lines of code

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

Vulnerability details

Impact

In the ProtocolDAO.upgradeExistingContract function, registerContract is called first and then unregisterContract is called.

	function upgradeExistingContract(
		address newAddr,
		string memory newName,
		address existingAddr
	) external onlyGuardian {
		registerContract(newAddr, newName);
		unregisterContract(existingAddr);
	}

If newName == oldName when upgradeExistingContract is called, then in the registerContract function, the address corresponding to name will be set to newAddr first

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

and when deleteAddress is called in the unregisterContract function, the newAddr corresponding to name will be deleted, so that the address corresponding to name will be 0.

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

Afterwards, it will be reverted in getContractAddress.

	function getContractAddress(string memory contractName) internal view returns (address) {
		address contractAddress = getAddress(keccak256(abi.encodePacked("contract.address", contractName)));
		if (contractAddress == address(0x0)) {
			revert ContractNotFound();
		}
		return contractAddress;
	}

Proof of Concept

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L209-L216 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L190-L203 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/BaseAbstract.sol#L90-L96

Tools Used

None

Recommended Mitigation Steps

Consider swapping the order of registerContract and unregisterContract calls in the upgradeExistingContract function

	function upgradeExistingContract(
		address newAddr,
		string memory newName,
		address existingAddr
	) external onlyGuardian {
-		registerContract(newAddr, newName);
		unregisterContract(existingAddr);
+              registerContract(newAddr, newName);
	}

Lines of code

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

Vulnerability details

Impact

The Staking.stakeGGP function has the whenNotPaused modifier, which means that when the contract is paused, it is not possible to stake the GGP with the stakeGGP function

	function stakeGGP(uint256 amount) external whenNotPaused {
		// Transfer GGP tokens from staker to this contract
		ggp.safeTransferFrom(msg.sender, address(this), amount);
		_stakeGGP(msg.sender, amount);
	}

But the restakeGGP function does not have the whenNotPaused modifier, which means that the user can call ClaimNodeOp.claimAndRestake to stake GGP when the contract is paused

	function restakeGGP(address stakerAddr, uint256 amount) public onlySpecificRegisteredContract("ClaimNodeOp", msg.sender) {
		// Transfer GGP tokens from the ClaimNodeOp contract to this contract
		ggp.safeTransferFrom(msg.sender, address(this), amount);
		_stakeGGP(stakerAddr, amount);
	}
...
	function claimAndRestake(uint256 claimAmt) external {
		Staking staking = Staking(getContractAddress("Staking"));
		uint256 ggpRewards = staking.getGGPRewards(msg.sender);
		if (ggpRewards == 0) {
			revert NoRewardsToClaim();
		}
		if (claimAmt > ggpRewards) {
			revert InvalidAmount();
		}

		staking.decreaseGGPRewards(msg.sender, ggpRewards);

		Vault vault = Vault(getContractAddress("Vault"));
		uint256 restakeAmt = ggpRewards - claimAmt;
		if (restakeAmt > 0) {
			vault.withdrawToken(address(this), ggp, restakeAmt);
			ggp.approve(address(staking), restakeAmt);
			staking.restakeGGP(msg.sender, restakeAmt);
		}

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider adding the whenNotPaused modifier to the Staking.restakeGGP function

Lines of code

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

Vulnerability details

Impact

In requireValidStateTransition, the recreateMinipool function can be called to create a minipool when the current state is Withdrawable/Error/Finished/Canceled

		} 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);
...
	function recreateMinipool(address nodeID) external whenNotPaused {
		int256 minipoolIndex = onlyValidMultisig(nodeID);
		requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);

The recreateMinipool function does not require the user to provide new avax, but uses the user's untaken avax. We also note that the withdrawMinipoolFunds function sets the status to Finished, which means that the recreateMinipool function can be called to create a minipool even if the remained avax is withdrawn

	function withdrawMinipoolFunds(address nodeID) external nonReentrant {
		int256 minipoolIndex = requireValidMinipool(nodeID);
		address owner = onlyOwner(minipoolIndex);
		requireValidStateTransition(minipoolIndex, MinipoolStatus.Finished);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Finished));

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

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

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

Consider the following scenario User A first provides 1000 avax and calls createMinipool to create a minipool with a duration of 14 days After 14 days, user A is rewarded with 4 avax. User A considers triggering recreateMinipool and calls the withdrawMinipoolFunds function before recreateMinipool is called. At this point User A has withdrawn 1004 avax and avaxStaked is 0

		staking.decreaseAVAXStake(owner, avaxNodeOpAmt);

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

In the recreateMinipool function, user A's avaxStaked is increased to 4 and avaxAssigned is increased to 1004, avaxNodeOpAmt == avaxLiquidStakerAmt == 1004

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

In claimAndInitiateStaking, the avaxStaked of user A is not checked and 1004 avax are taken directly from the Vault

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

After 14 days, user A gets 4 avax rewards, in withdrawMinipoolFunds, at this time avaxNodeOpAmt == 1004, and avaxStaked == 4, in the call decreaseAVAXStake will fail due to overflow, but as long as the user creates another minipool with 1000 avax at this time, avaxStaked will increase to 1004, at this time call withdrawMinipoolFunds will successfully take out 1008 avax.

	function withdrawMinipoolFunds(address nodeID) external nonReentrant {
		int256 minipoolIndex = requireValidMinipool(nodeID);
		address owner = onlyOwner(minipoolIndex);
		requireValidStateTransition(minipoolIndex, MinipoolStatus.Finished);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Finished));

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

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

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

In the second Staking, user A does not provide any avax and gets a double reward( 8 == 4*2)

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider using a new state to ensure that recreateMinipool cannot be called after withdrawMinipoolFunds

Lines of code

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

Vulnerability details

Impact

When a NodeOP cannot guarantee 80% reliability, it will not be rewarded with avax, and in MinipoolManager.recordStakingEnd, the NodeOP will be slashed

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

In the slash function, the slash amount is 10% of avaxLiquidStakerAmt

         setUint(keccak256("ProtocolDAO.ExpectedAVAXRewardsRate"), 0.1 ether); // Annual rate as pct of 1 avax
...
	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);
	}

But considering that the minimum required collateral rate for a NodeOP to create a minipool is 10% of avaxAssigned, and that avaxAssigned == avaxLiquidStakerAmt in general

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

if the price of GGP drops during the time between the creation of the minipool and the slash, the amount of GGPs collateralized by the user will be less than the amount slashed, and the decreaseGGPStake function will fail due to overflow in the slashGGP function, thus causing the slash to fail

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

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Consider the user's GGP balance in the slashGGP function

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

Lines of code

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

Vulnerability details

Impact

The documentation says

NodeOps are eligible for a reward cycle if they have had at least 1 active minipool for a set amount of time, currently 14 days.

In the ClaimNodeOp.isEligible function, the RewardsStartTime is used to confirm whether NodeOps are eligible for GGP rewards

	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()); //  >= 14 days
	}

In the MinipoolManager contract, RewardsStartTime is set when the minipool is created

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

However, in the recordStakingError and _cancelMinipoolAndReturnFunds functions, when the minipool is cancelled, or when Staking error (the minipool is not closed due to the end of Staking, i.e. the minipool was not active), it does not reset the RewardsStartTime

	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);
	}
...
	function recordStakingError(address nodeID, bytes32 errorCode) external payable {
		int256 minipoolIndex = onlyValidMultisig(nodeID);
		requireValidStateTransition(minipoolIndex, MinipoolStatus.Error);

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

		if (msg.value != (avaxNodeOpAmt + avaxLiquidStakerAmt)) {
			revert InvalidAmount();
		}

		setBytes32(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".errorCode")), errorCode);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Error));
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxTotalRewardAmt")), 0);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpRewardAmt")), 0);
		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerRewardAmt")), 0);

		// Send the nodeOps AVAX to vault so they can claim later
		Vault vault = Vault(getContractAddress("Vault"));
		vault.depositAVAX{value: avaxNodeOpAmt}();

		// Return Liq stakers funds
		ggAVAX.depositFromStaking{value: avaxLiquidStakerAmt}(avaxLiquidStakerAmt, 0);

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

		subUint(keccak256("MinipoolManager.TotalAVAXLiquidStakerAmt"), avaxLiquidStakerAmt);

		emit MinipoolStatusChanged(nodeID, MinipoolStatus.Error);
	}

In the calculateAndDistributeRewards function of the ClaimNodeOp contract, this means that the user will be rewarded with GGP even if there is no active minipool for that user.

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

Proof of Concept

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L46-L52 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L484-L515 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L646-L665 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L56-L85

Tools Used

None

Recommended Mitigation Steps

In the recordStakingError and _cancelMinipoolAndReturnFunds functions, when minipool counts to 0, reset RewardsStartTime (need to decrease minipool count in recordStakingError, in a separate report)

Lines of code

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

Vulnerability details

Impact

In MinipoolManager, when a minipool is created, the minipool count is increased, and when Staking is ended or the minipool is canceled, the minipool count is decreased. However, in the recordStakingError function, the minipool count does not decrease if there is a Staking creation error.

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

		subUint(keccak256("MinipoolManager.TotalAVAXLiquidStakerAmt"), avaxLiquidStakerAmt);

This means that the minipool count of the user with Staking creation error will never be 0. In the calculateAndDistributeRewards function of the ClaimNodeOp contract, this means that the user will be rewarded with GGP even if there is no active minipool for that user.

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

Proof of Concept

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

Tools Used

None

Recommended Mitigation Steps

Decrease the minipool count in MinipoolManager.recordStakingError

		Staking staking = Staking(getContractAddress("Staking"));
		staking.decreaseAVAXAssigned(owner, avaxLiquidStakerAmt);
+               staking.decreaseMinipoolCount(owner);
		subUint(keccak256("MinipoolManager.TotalAVAXLiquidStakerAmt"), avaxLiquidStakerAmt);