M-02.md

Minipool state inconsistency during error transition

The function recordStakingError present in the MinipoolManager contract is used to finalize a staking cycle in the case of an error. This function fails to properly update the minipool counter for the node operator.

Impact

When the node operator create a minipool using the createMinipool function a call to Staking.increaseMinipoolCount is used to register this new minipool:

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

staking.increaseMinipoolCount(msg.sender);

The function recordStakingEnd will properly decrease this counter (line 437). However, recordStakingError doesn't call Staking.decreaseMinipoolCount to balance the counter accordingly. This will leave an inconsistency in the counter every time the staking cycle is ended with an error using the recordStakingError as the counter isn't decremented.

Note that this also can happen from the Finished state, since after a review of the error the finishFailedMinipoolByMultisig will move the state from the Error status to the Finished status.

PoC

The following test shows how the counter is incorrect after the Rialto multisig register the staking error.

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.17;

import "./utils/BaseTest.sol";
import {FixedPointMathLib} from "@rari-capital/solmate/src/utils/FixedPointMathLib.sol";

contract AuditTest is BaseTest {
	using FixedPointMathLib for uint256;
	address private nodeOp;

	function setUp() public override {
		super.setUp();
		nodeOp = getActorWithTokens("nodeOp", MAX_AMT, MAX_AMT);
	}
	
	function test_MinipoolManager_StateInconsistency() public {
		// Setup minipool and get to the error state by following the normal steps
		address liqStaker = getActorWithTokens("liqStaker", MAX_AMT, MAX_AMT);
		vm.prank(liqStaker);
		ggAVAX.depositAVAX{value: MAX_AMT}();

		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;

		vm.startPrank(nodeOp);

		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(100 ether);
		MinipoolManager.Minipool memory mp = createMinipool(depositAmt, avaxAssignmentRequest, duration);

		vm.stopPrank();

		assertEq(staking.getMinipoolCount(nodeOp), 1);

		vm.startPrank(address(rialto));

		minipoolMgr.claimAndInitiateStaking(mp.nodeID);
		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(mp.nodeID, txID, block.timestamp);

		bytes32 errorCode = keccak256("errorCode");
		minipoolMgr.recordStakingError{value: validationAmt}(mp.nodeID, errorCode);

		vm.stopPrank();

		// Count is still 1 for nodeOp
		assertEq(staking.getMinipoolCount(nodeOp), 1);
	}
}

Recommendation

The recordStakingError function should also decrease the minipool count:

function recordStakingError(address nodeID, bytes32 errorCode) external payable {
 	...
 	
 	Staking staking = Staking(getContractAddress("Staking"));
 	staking.decreaseAVAXAssigned(owner, avaxLiquidStakerAmt);
+	staking.decreaseMinipoolCount(owner);
 	...
}