In cancelMinipool() code should check wait period based on creation time of the minipool not user's reward start time.

[code423n4]

Lines of code

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

Vulnerability details

Impact

According to the doc "A minipool can only be canceled by the owner after a 5 day waiting period. This is to prevent griefing." (https://docs.gogopool.com/design/minipooldesign#canceled) but current implementation in the code would allows for node operators to create and cancel Minipools immediately. by exploiting this issue attackers can perform various malicious actions, the first is griefing, attacker can lock liquidity stakers AVAX and prevent other Minipools from using those AVAX tokens and when multising wants to create a minipool attacker can front-run and cancel the Minipool. the second scenario is that attacker can create minipool and set the value of rewardsStartTime for his address and then cancel the minipool and use that AVAX to create minipool for his other addresses set rewardsStartTime for those too. (setting the value of rewardsStartTime for multiple addresses with same AVAX by creating and cancelling Minipools all in one transaction). the other scenario is that attacker can prevent other node runners from creating Minipools by performing sandwich attack (create a minipool with that nodeId before node runner transaction and then node runner transaction would fail and then cancel the minipool). so this issue can have multiple impacts and attacker can broke basic functionality of the protocol which is creating minipool and using liqudity stakers funds and earning rewards.

Proof of Concept

This is cancelMinipool() code:

    function cancelMinipool(address nodeID) external nonReentrant {
        Staking staking = Staking(getContractAddress("Staking"));
        ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
        int256 index = requireValidMinipool(nodeID);
        onlyOwner(index);
        // make sure they meet the wait period requirement
        if (block.timestamp - staking.getRewardsStartTime(msg.sender) < dao.getMinipoolCancelMoratoriumSeconds()) {
            revert CancellationTooEarly();
        }
        _cancelMinipoolAndReturnFunds(nodeID, index);
    }

As you can see to check that wait period has been passed code uses block.timestamp - staking.getRewardsStartTime(msg.sender) < dao.getMinipoolCancelMoratoriumSeconds() and code assumes staking.getRewardsStartTime(msg.sender) is the creation time of the Minipool but that is not the creation time of this Minipool and can be creation time of user's first Minipool. so This bug can make attacker to create a Minipool and set the value of the staking.getRewardsStartTime(msg.sender) for his address and then after 5 days attacker can create and cancel minipools instantly and by this attacker can cause all sort of greifing for liquidity stakers and node runners and can break fundemental functionality of the protocol. scenario 1: attacker can prevent others from creating minipools by performing sandwich attack

1. attacker would wait for node runners createMinipool() transaction to get added to mempools.
2. attacker would create a create minipool transaction with higher gas price and with the same nodeId as user transaction and with 1000 AVAX.
3. attacker transaction would execute first and change the state of that nodeId and then user transaction would fail because that nodeId already in the prelunch state.
4. then attacker can cancel that minipool with another transaction (can be done in the same transaction).

by performing this attacker can prevent any minipool creation (by cooperating with mining pools attacker can make all the createMinipool() transactions to fail just by setting order of the transaction and only with 1000 AVAX and 10% ggp collateral). this would cause main functionality of the protocol to fail and node runners can't create minipools and liquidity stakers funds can't be used in node validations and no rewards would be generates for them.

attacker can perform other attacks too and exploit the instant minipool creation and cancellation without any penalty. for example by creating and cancelling minipool for his multiple address can set rewardsStartTime for his addresses while circulating same AVAX fund and in the end all his addresses would have rewardsStartTime and would be eligible for rewards while none of them has active minipools.

Tools Used

VIM

Recommended Mitigation Steps

store start time of each miniPool and use it for calculating passed time.

[c4-judge]

GalloDaSballo marked the issue as duplicate of #519

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo marked the issue as satisfactory