A node operator’s minipoolCount could go wrong when the state is ERROR

[code423n4]

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.

[0xminty]

dupe of #235

[c4-judge]

GalloDaSballo marked the issue as duplicate of #235

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)