Canceled miniPools that are recreated will drain the contract.

[code423n4]

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.

[c4-judge]

GalloDaSballo marked the issue as duplicate of #484

[GalloDaSballo]

Dup of #484 without front-run, awarding half

[c4-judge]

GalloDaSballo marked the issue as partial-50

[c4-judge]

GalloDaSballo marked the issue as duplicate of #569

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[Simon-Busch]

Changed severity back to M as requested by @GalloDaSballo