GoGoPool contest - Manboy's results

Lines of code

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

Vulnerability details

Impact

If a minipool is in the Withdrawable or Error state (MinipoolStatus), anyone can create a new minipool with the same nodeID (using the createMinipool function). This will prevent the original minipool owner from withdrawing their funds (via withdrawMinipoolFunds) since they will no longer be the owner. The funds will be locked in the contract.

Proof of Concept

At line 206 in MinipoolManagerTest (in the testWithdrawMinipoolFunds function), add:

address hacker = getActorWithTokens("hacker", MAX_AMT, MAX_AMT);
vm.startPrank(hacker);
ggp.approve(address(staking), MAX_AMT);
staking.stakeGGP(ggpStakeAmt);

uint256 delegationFee = 0.02 ether;
minipoolMgr.createMinipool{value: depositAmt}(mp1.nodeID, 2 weeks, delegationFee, 1000 ether);
vm.stopPrank();

This code snippet will create a new minipool with the same nodeID as nodeOp's minipool and make the nodeOp's subsequent withdrawal attempt fail.

Tools Used

Foundry

Recommended Mitigation Steps

In the requireValidStateTransition function, the transition from Withdrawable or Error states to the Prelaunch state should not be valid.

Lines of code

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

Vulnerability details

Impact

After the finishFailedMinipoolByMultisig function is called (by a valid mutisig) on a minipool in the Error or Withdrawable state, the node operator will no longer be able to withdraw (via the withdrawMinipoolFunds function) from that minipool.

This is because the finishFailedMinipoolByMultisig function will set the minipool into the Finished state, and in the withdrawMinipoolFunds function, the call to the requireValidStateTransition function will fail, as the transitioning from the Finished state to the Finished state is not a valid state transition.

Also note that the finishFailedMinipoolByMultisig function does not check that the minipool is in the Error state, so it can also be called on a minipool in the Withdrawable state, as Withdrawable -> Finished is a valid state transition. Preventing the node operator from withdrawing.

Proof of Concept

After line 577 in test/unit/MinipoolManager.t.sol (at the end of the testRecordStakingError function), add this code snippet:

// the nodeOp tries to withdraw after the minipool went into the `Error` state, and the `finishFailedMinipoolByMultisig` 
// function was called by the multisig
vm.prank(nodeOp);
minipoolMgr.withdrawMinipoolFunds(mp1.nodeID);

This will fail with an InvalidStateTransition error, as the state change from Finished to Finished in the withdrawMinipoolFunds function fails.

Tools Used

Foundry

Recommended Mitigation Steps

Remove the finishFailedMinipoolByMultisig function. If required, add a makeWithdrawableFailedMinipoolByMultisig, that lets the multisig transition the minipool state from the Error to then Withdrawable state. This will still allow the node operator to withdraw.