recreateMinipool() could lead to losing funds

[code423n4]

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()

[GalloDaSballo]

Basically dup of #569 with some extra info

[0xju1ie]

Case 1 depends on Rialto making a mistake. It would not call recreate() on a minipool that had never staked. And we are assuming for the audit that Rialto is a perfect actor so this is invalid.

Case 2 I do not really understand... Seems like it is working as intended. Our UI will prevent anything that is not in 14day increments, but even if that wasnt there the staking would fail and recordStakingError() would be called after claimAndInitiateStaking().

[c4-judge]

GalloDaSballo marked the issue as duplicate of #569

[c4-judge]

GalloDaSballo marked the issue as partial-50

[GalloDaSballo]

Basically issues with validation that can lead to issues but unclear -> awarding 50% because the FSM / check is incorrect but is not as accurate as 569

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[Simon-Busch]

Changed the severity back to M as requested by @GalloDaSballo