GoGoPool contest - wagmi's results

Lines of code

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

Vulnerability details

Impact

AVAXAssignedHighWater variable is the highest amount of AVAX a Node Operator was assigned during a given rewards period. A minipool can end in the middle of a reward cycle, this variable is used to make sure stakers still get rewarded, even though at the time rewards are distributed they might not have a minipool.

Basically it should be the highest amount of AVAX assigned, when it found that current assigned amount is larger than AVAXAssignedHighWater, AVAXAssignedHighWater should be set to current assigned amount. However, the current logic is wrong, it increased the amount instead of just set it to current amount.

if (staking.getAVAXAssignedHighWater(owner) < staking.getAVAXAssigned(owner)) {
    staking.increaseAVAXAssignedHighWater(owner, avaxLiquidStakerAmt);
}

So if stakers has more than 1 minipool, they could receive more reward than they should.

Proof of Concept

Consider the scenario

1. Alice has 2 minipool with pool1.AVAXAssigned = 1000 AVAX and pool2.AVAXAssigned = 2000 AVAX.
2. When multisig call recordStakingStart() for pool1, AVAXAssignedHighWater = 0 < 1000 AVAX so it is increased by 1000 AVAX. So now AVAXAssignedHighWater = 1000 AVAX.
3. For pool2, Alice is assigned 2000 AVAX more, so staking.getAVAXAssigned(Alice) = 3000. And again 1000 < 3000, so AVAXAssignedHighWater is increased by 3000 AVAX.
4. At the end AVAXAssignedHighWater = 4000 AVAX while it should only at most 1000 + 2000 = 3000 AVAX.

Tools Used

Manual Review

Recommended Mitigation Steps

Set AVAXAssignedHighWater to current amount instead of increasing it.

Lines of code

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

Vulnerability details

Impact

Each minipool has a state and a set of states that it can move to. In the codebase, state ERROR and WITHDRAWABLE is allowed to move to state PRELAUNCH.

function requireValidStateTransition(int256 minipoolIndex, MinipoolStatus to) private view {
    ...
    } else if (currentStatus == MinipoolStatus.Withdrawable || currentStatus == MinipoolStatus.Error) {
        isValid = (to == MinipoolStatus.Finished || to == MinipoolStatus.Prelaunch);
    }
    ...
}

As we can see, when minipool in state ERROR or WITHDRAWABLE, owner of the minipool can call withdrawMinipoolFunds() to claim all AVAX funds from Vault. However, since it is allowed to move to state PRELAUNCH, attacker can call createMinipool() with nodeId of existing minipool which is in state ERROR for example.

In createMinipool(), if minipool exists, it will reset all param of minipool and set new owner

function createMinipool(
    address nodeID,
    uint256 duration,
    uint256 delegationFee,
    uint256 avaxAssignmentRequest
) external payable whenNotPaused {
    ...
    if (minipoolIndex != -1) {
        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);
    }
    ...
    setAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".owner")), msg.sender);
    ...
}

The result is the owner of that minipool will be overriden to be attacker address and victim funds (unclaimed funds) is lost.

Proof of Concept

Consider the scenario:

1. Alice create a minipool with avaxAssignmentRequest = 2000 AVAX.
2. During validation, Alice's node operator has some errors and Rialto call recordStakingError() to return Alice and liquid stakers' AVAX.
3. Before Alice call withdrawMinipoolFunds(), Bob (attacker) calls createMinipool() with nodeId is Alice's minipool nodeId.
4. All the check will pass, Bob becomes owner of that minipool, and now Alice calling withdrawMinipoolFunds() will fail.

Tools Used

Manual Review

Recommended Mitigation Steps

Do not allow moving to state PRELAUNCH from state that fund is not claimed.

Judge has assessed an item in Issue #653 as 2 risk. The relevant finding follows:

1. Funds are locked if Rialto use function finishFailedMinipoolByMultisig() https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L528

Detail Function finishFailedMinipoolByMultisig() did not transfer any funds or doing any data change, only updating state of minipool to Finished. In the comment, it said that it used to finish error pool. However, if minipool is at state Error, it still has AVAX stored in Vault. If it just update state to Finished, this amount of AVAX is locked and cannot return to owner.

Even though this issue is loss of funds but since it can only happen by Rialto calling so I put it in Low.

Recommendation Allowing Rialto to withdraw funds and maybe return to owner later.

Lines of code

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

Vulnerability details

Impact

When doing inflation, function getInflationAmt() calculated number of intervals elapsed by dividing the duration with interval length.

function getInflationIntervalsElapsed() public view returns (uint256) {
    ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
    uint256 startTime = getInflationIntervalStartTime();
    if (startTime == 0) {
        revert ContractHasNotBeenInitialized();
    }
    return (block.timestamp - startTime) / dao.getInflationIntervalSeconds();
}

As we can noticed that, this calculation is rounding down, it means if block.timestamp - startTime = 1.99 intervals, it only account for 1 interval.

However, when updating start time after inflating, it still update to current timestamp while it should only increased by intervalLength * intervalsElapsed instead.

addUint(keccak256("RewardsPool.InflationIntervalStartTime"), inflationIntervalElapsedSeconds); 
// @audit should only update to oldStartTime + inverval * numInterval
setUint(keccak256("RewardsPool.RewardsCycleTotalAmt"), newTokens);

Since default value of inflation interval = 1 days and reward cycle length = 14 days, so the impact is reduced. However, these configs can be changed in the future.

Proof of Concept

Consider the scenario:

1. Assume last inflation time is InflationIntervalStartTime = 100. InflationIntervalSeconds = 50.
2. At timestamp = 199, function getInflationAmt() will calculate

inflationIntervalsElapsed = (199 - 100) / 50 = 1
// Compute inflation for total inflation intervals elapsed
for (uint256 i = 0; i < inflationIntervalsElapsed; i++) {
    newTotalSupply = newTotalSupply.mulWadDown(inflationRate);
} // @audit only loop once.

3. And then in inflate() function, InflationIntervalStartTime is still updated to current timestamp, so InflationIntervalStartTime = 199.
4. If this sequence of actions are repeatedly used, we can easily see

InflationIntervalStartTime = 199, inflated count = 1
InflationIntervalStartTime = 298, inflated count = 2
InflationIntervalStartTime = 397, inflated count = 3
InflationIntervalStartTime = 496, inflated count = 4
InflationIntervalStartTime = 595, inflated count = 5

While at timestamp = 595, inflated times should be (595 - 100) / 50 = 9 instead.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider only increasing InflationIntervalStartTime by the amount of intervals time interval length.

Lines of code

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

Vulnerability details

Impact

From Discord channel

To clarify duration for everyone -- a nodeOp can choose a duration they want, from 14 days to 365 days. But behind the scenes, Rialto will only create a validator for 14 days. At the end of the 14 days, the Avalanche protocol pays out the validation rewards and all funds are returned to the contracts, which divvy up the rewards between liq stakers and nodeops. If the duration still has time left to go, Rialto will call recreateMinipool which will do another 14 days cycle( if it can considering GGP collat and available liq staker funds), compounding the rewards. In this way, the protocol recvs yield from every minipool every 14 days, and the collat ratio of GGP can never get more than 14 days out of whack. In the future we see a path for laddering maturities to maximize yield and still provide necessary liq for stakers.

So basically, Rialto calls recordStakingEnd() then recreateMinipool() to compound the rewards for owners. After calling recordStakingEnd(), minipool is at Withdrawable state.

Because of that, attacker (owner of minipool) can front-run recreateMinipool(), calling withdrawMinipoolFunds(), withdraw the funds. Interestingly, function withdrawMinipoolFunds() did not reset any data of minipool (avaxNodeOpAmt and avaxNodeOpRewardAmt are not reset) and state is moved to Finished.

However, Finished -> Prelaunch is possible so recreateMinipool() will not fail and still create the minipool and increase AVAX stake of owner.

Proof of Concept

Consider the scenario

1. Alice create a minipool with duration = 365 days with 1000 AVAX
2. After first 14 days, Rialto calls recordStakingEnd() then call recreateMinipool() to compound rewards for Alice.
3. Alice front-run second TXs (the one call recreateMinipool()) and call withdrawMinipoolFunds(). Alice get 1000 AVAX back.
4. Second TX of Rialto passed, minipool is still created and Alice get a 1000 AVAX minipool without depositing any funds back. Of courses, after that, Alice can choose to cancel the minipool to get 1000 AVAX.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider resetting data when withdrawing minipool funds.

Lines of code

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

Vulnerability details

Impact

For each owner, there is a wait period requirement before they can cancel their minipool after creation.

if (block.timestamp - staking.getRewardsStartTime(msg.sender) < dao.getMinipoolCancelMoratoriumSeconds()) {
    revert CancellationTooEarly(); 
    // @audit Minipool can be cancelled immediately after creation if owner has more than 1 pool
}

However, as we can see, it used rewards start time of owner to check this wait period requirement. If owner has more than 1 pool, the second, third,... pools can be cancelled immediately after creation because duration from first pool creation to current timestamp is enough to pass minipoolCancelMoratoriumSeconds.

Proof of Concept

Consider the scenario

1. Alice created a minipool A at t1 = 100. Rewards start time is recorded at that momemnt rewardsStartTime = 100.
2. Alice created a minipool B at t2 = 200. Since 200 - rewardsStartTime = 100 already so Alice can cancelled minipool B immediately after its creation.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider using another field to store creation timestamp of minipool.

Lines of code

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

Vulnerability details

Impact

GGP staked is used to compensate liquid stakers in case minipool did not receive AVAX reward (low uptime). However, price of GGP at staking time and at slashing time might be different, for example, price might get lower.

function slash(int256 index) private {
    address nodeID = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".nodeID")));
    address owner = getAddress(keccak256(abi.encodePacked("minipool.item", index, ".owner")));
    uint256 duration = getUint(keccak256(abi.encodePacked("minipool.item", index, ".duration")));
    uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt")));
    uint256 expectedAVAXRewardsAmt = getExpectedAVAXRewardsAmt(duration, avaxLiquidStakerAmt);
    uint256 slashGGPAmt = calculateGGPSlashAmt(expectedAVAXRewardsAmt);
    setUint(keccak256(abi.encodePacked("minipool.item", index, ".ggpSlashAmt")), slashGGPAmt);

    emit GGPSlashed(nodeID, slashGGPAmt);

    Staking staking = Staking(getContractAddress("Staking"));
    staking.slashGGP(owner, slashGGPAmt); 
    // @audit cannot slash if GGP staked is less than slashGGPAmt
}

In that case, slashGGPAmt might increase when it get slashed. And instead of slash as much as possible, for example, slash all available amount of owner, it fail and revert the transaction.

Minipool cannot be slashed, it means liquid stakers did not get funds back and node operator AVAX is locked too.

Proof of Concept

Consider the scenario:

1. Alice has a minipool with 1000 AVAX - 1000 AVAX Assigned. At this moment, 1 AVAX = 1 GGP and Alice stake 100 GGP.
2. During staking period, Alice node does not has enough uptime and did not get any reward. 100 GGP should be slashed to compensate liquid staker reward from 1000 AVAX.
3. However, now 1 AVAX = 2 GGP, so the slashGGPAmt is calculated to be 200 GGP. Instead of get as much as possible to compenstate (100 GGP), TX fail when trying to slash 200 GGP.

function slashGGP(address stakerAddr, uint256 ggpAmt) public onlySpecificRegisteredContract("MinipoolManager", msg.sender) {
    Vault vault = Vault(getContractAddress("Vault"));
    decreaseGGPStake(stakerAddr, ggpAmt);
    vault.transferToken("ProtocolDAO", ggp, ggpAmt);
}

Tools Used

Manual Review

Recommended Mitigation Steps

Consider slashing as much as possible in case there is not enough GGP in owner balance.

Lines of code

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

Vulnerability details

Impact

Minipool count of each staker address is used ClaimNodeOp to reset rewards start time in case there is no minipool running. However, the accouting logic in MinipoolManager forgot to decrease minipoolCount in recordStakingError() function.

function recordStakingError(address nodeID, bytes32 errorCode) external payable {
    ...
    // Return Liq stakers funds
    ggAVAX.depositFromStaking{value: avaxLiquidStakerAmt}(avaxLiquidStakerAmt, 0);

    Staking staking = Staking(getContractAddress("Staking"));
    staking.decreaseAVAXAssigned(owner, avaxLiquidStakerAmt); // @audit forget to decrease minipool count ?
    ...
}

As the result, even when there is no minipool, rewards start time of staker is not reset, staker will get free reward.

// check if their rewards time should be reset
uint256 minipoolCount = staking.getMinipoolCount(stakerAddr);
if (minipoolCount == 0) { 
    // @audit wrong minipool count, reward time is not reset
    staking.setRewardsStartTime(stakerAddr, 0);
}

Proof of Concept

Consider the scenario:

1. Alice created a minipool using createMinipool() function, minipoolCount is increased to 1.
2. During staking period, there is an error and function recordStakingError() is called, minipoolCount still has value of 1.
3. Alice called createMinipool() again with the same nodeId, it still call staking.increaseMinipoolCount(msg.sender); normally, so as the result minipoolCount = 2 while Alice only has 1 minipool actually
4. Now even if this minipool is cancelled or finished, Alice still has minipoolCount = 1 and rewards start time is not reset.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider decreasing minipoolCount in case minipool has error.