GoGoPool contest - HollaDieWaldfee's results

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L279-L281 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L56-L85 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L163-L167 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L223

Vulnerability details

Impact

This report deals with how a staker can manipulate his avaxAssignedHighWater in order to earn higher GGP rewards.

In the "known issues" of this contest (https://multisiglabs.notion.site/Known-Issues-42e2f733daf24893a93ad31100f4cd98) there is a section that deals with GGP rewards manipulation, stating that:

Just before rewards are distributed, a NodeOp can increase their GGP stake to claim a bigger share of rewards, even if they have no minipools currently running.

The issue I am explaining here is a different one however. The known issue deals with increasing the GGP stake just before rewards are distributed up to the maximum 150% collateralization ratio.

I am describing how the avaxAssignedHighWater can be manipulated, thereby increasing the amount of GGP needed to reach the 150% collateralization ratio.

Therefore if avaxAssignedHighWater is manipulated to be higher than it should be, a malicious staker can stake even more GGP and earn even higher rewards.

The issue exists because if a staker creates a first Minipool and waits MinipoolCancelMoratoriumSeconds, he can cancel new Minipools instantly (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L279-L281). RewardsStartTime is set when the first Minipool is created and will not be set again when a second Minipool is created.

The issue is that ClaimNodeOp.calculateAndDistributeRewards (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L56-L85) calls staking.resetAVAXAssignedHighWater which sets avaxAssignedHighWater to the amount of AVAX currently assigned (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L163-L167).

Proof of Concept

1. The staker creates Minipool A
2. The staker waits 14 days (now he is eligible for rewards and can cancel new Minipools at any time)
3. The multisig calls ClaimNodeOp.calculateAndDistributeRewards which calls staking.resetAVAXAssignedHighWater which resets avaxAssignedHighWater to the amount of currently assigned AVAX
4. The staker front-runs this transaction and creates a Minipool B with a LOT of AVAX
5. After the rewards are distributed the staker cancels the Minipool B
6. Now the staker's avaxAssignedHighWater is set to the very high value and the staker has a very high ceiling of rewards he can earn the next time the claimNodeOp.calculateAndDistributeRewards function is called

Tools Used

VSCode

Recommended Mitigation Steps

There is no quick fix for this because obviously avaxAssignedHighWater should be adapted in ClaimNodeOp.calculateAndDistributeRewards. The only value that it can be set to is the currently assigned AVAX.

I propose to change how avaxAssigned is increased. Currently avaxAssigned is increased when the Minipool is created (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L223).

I think it is better to increase it in the MinipoolManager.claimAndInitiateStaking function. Because at this point, the staker can no longer cancel the Minipool.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L385-L440 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L444-L478 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L424-L426 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L430 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L424-L426 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L673 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L674

Vulnerability details

Impact

According to a message by the sponsor in the Discord channel (https://discord.com/channels/810916927919620096/1052967326502363146/1058480285169238147), AVAX is restaked every 14 days.
This means that even when a node operator provides a duration of say 28 days, the staking will be ended after 14 days using the MinipoolManager.recordStakingEnd function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L385-L440) and the Minipool will be recreated using the MinipoolManager.recreateMinipool function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L444-L478).

In the Minipool.recreateMinipool function, the rewards of the previous staking period are compounded. The duration is not updated. It stays at 28 days for the whole lifetime of the Minipool.

The way slashing is implemented introduces an issue.

Say the duration of a Minipool is 140 days (i.e. 10 staking periods of 14 days each). Let's assume that staking fails in the tenth staking period i.e. no rewards are sent to MinipoolManager.recordStakingEnd. This will trigger the slashing mechanism (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L424-L426).

Slashing occurs with the compounded amount which is considerably higher than the initially staked AVAX amount. And the duration for which to slash GGP will be the whole 140 days duration.

Obviously this is wrong. The previous 9 staking periods were successful and rewards were paid to TokenggAVAX holders. So the slashing should only occur for the tenth staking period.

Proof of Concept

1. A node operator creates a Minipool with a duration of 140 days
2. 9 staking periods (i.e. 126 days) pass successfully and AVAX rewards are paid to TokenggAVAX holders (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L430)
3. The tenth staking period fails so the slash function is executed (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L424-L426). It calculates the amount to slash for the whole 140 days duration (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L673) and based on the compounded amount of AVAX staked (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L674)

In this example the amount that was actually slashed was ten times as much as it should have been.

Tools Used

VSCode

Recommended Mitigation Steps

I cannot think of an easy fix for how to implement slashing correctly because the implementation of the Rialto Multisig that manages the Minipools is not known to me. So I am not able to say how a proposed fix is / is not compatible with the Rialto Multisig.

I think though that a possible solution can be to hard-code the 14 day slashing period. Such that when slashing is executed, it always slashes 14 days of GGP rewards.

Alternatively, the Rialto Multisig might provide the optional slashing duration to the MinipoolManager.recordStakingEnd function just like it provides totalEligibleGGPStaked to the ClaimNodeOp.calculateAndDistributeRewards.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L196-L269 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L243 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L152-L175 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L279-L281

Vulnerability details

Impact

The MinipoolManager.createMinipool function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L196-L269) is used to create minipools. It can also be used to overwrite existing nodeIDs.

The only requirement in order to overwrite an existing nodeID is (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L243):

requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);

You can see in the MinipoolManager.requireValidStateTransition function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L152-L175) that the state can change into the Prelaunch state from 4 states:
Withdrawable, Error, Finished and Canceled.

There are two checks missing for overwriting an existing nodeID:

Firstly, the state of the Minipool must not be Withdrawable. Overwriting a Withdrawable Minipool causes the rewards that are yet to be withdrawn to be lost.

Secondly I think the minipool should only be overwritten by the owner. Because if anyone can do it, then the owner that wants to overwrite the nodeID may not get the chance to because the nodeID is already overwritten.

Another important thing that is important to explain in order to describe a practical exploitation scenario is that if a staker creates one Minipool, then waits MinipoolCancelMoratoriumSeconds, he can create a new Minipool and cancel that new Minipool instantly (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L279-L281).

The exploitation scenario I will detail in the "Proof of Concept" section allows a malicious staker to reset the rewards of other stakers' minipools that are in the Withdrawable state.

Furthermore because the malicious staker has created a valid Minipool and waited MinipoolCancelMoratoriumSeconds, he can cancel new Minipools at any time, which allows him to perform the attack without any risk of freezing his funds by getting them staked.

Proof of Concept

I created the following test file that shows how a malicious staker A can overwrite the Minipool of another staker B that is in the Withdrawable state, thereby resetting the rewards which leads to a loss for staker B.

Put the test into the test/unit folder and run it with forge test --match testCreateMinipoolOtherStakerMalicious -vv.

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity 0.8.17;

import "./utils/BaseTest.sol";
import {FixedPointMathLib} from "@rari-capital/solmate/src/utils/FixedPointMathLib.sol";

contract MinipoolManagerTest is BaseTest {
	using FixedPointMathLib for uint256;
	int256 private index;
	address private nodeOp;
	uint256 private status;
	uint256 private ggpBondAmt;

	function setUp() public override {
		super.setUp();
		nodeOp = getActorWithTokens("nodeOp", MAX_AMT, MAX_AMT);
	}

	function testCreateMinipoolOtherStakerMalicious() public {
		uint256 duration = 2 weeks;
		uint256 depositAmt = 1000 ether;
		uint256 avaxAssignmentRequest = 1000 ether;
		uint256 validationAmt = depositAmt + avaxAssignmentRequest;
		uint128 ggpStakeAmt = 200 ether;
		uint256 delegationFee = 0.02 ether;
		address nodeID = address(0x69);

		address nodeOp2 = getActorWithTokens("nodeOp2", MAX_AMT, MAX_AMT);

		// nodeOp stakes
		vm.startPrank(nodeOp);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(ggpStakeAmt);
	
		minipoolMgr.createMinipool{value: depositAmt}(nodeID, duration, delegationFee, avaxAssignmentRequest);
		int256 mPindex = minipoolMgr.getIndexOf(nodeID);
		MinipoolManager.Minipool memory mp1 = minipoolMgr.getMinipool(mPindex);
		vm.stopPrank();
	
		// start staking
		address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
		vm.prank(liqStaker1);
		ggAVAX.depositAVAX{value: MAX_AMT}();

		vm.prank(address(rialto));
		minipoolMgr.claimAndInitiateStaking(mp1.nodeID);

		bytes32 txID = keccak256("txid");
		vm.prank(address(rialto));
		minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);

		skip(duration);

		vm.startPrank(address(rialto));
		uint256 rewards = 10 ether;
		uint256 halfRewards = rewards / 2;
		deal(address(rialto), address(rialto).balance + rewards);
		// rialto multisig reports staking end
		minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp1.nodeID, block.timestamp, rewards);
		vm.stopPrank();

		int256 minipoolIndex = minipoolMgr.getIndexOf(mp1.nodeID);
		MinipoolManager.Minipool memory mp1Updated = minipoolMgr.getMinipool(minipoolIndex);
		console.log(mp1Updated.avaxTotalRewardAmt);
		console.log(mp1Updated.owner);
		// nodeOp2 stakes
		vm.startPrank(nodeOp2);
		ggp.approve(address(staking), MAX_AMT);
		staking.stakeGGP(ggpStakeAmt);
		minipoolMgr.createMinipool{value: 1000 ether}(nodeID, 300 days, delegationFee, 1000 ether);

		MinipoolManager.Minipool memory mp1Updated2 = minipoolMgr.getMinipool(minipoolIndex);
		console.log(mp1Updated2.avaxTotalRewardAmt);
		console.log(mp1Updated2.owner);
	}
}

The practical exploitation scenario which resets the rewards for the stakers looks like this:

1. The malicious staker A creates a minipool A with 1000 AVAX Stake (minimum amount)
2. Staker A waits 5 days. This means he can cancel new Minipools instantly now
3. Staker A waits for Minipools of other stakers to become Withdrawable
4. Staker A calls createMinipool with the Withdrawable Minipool's nodeID to reset the reward
5. Within the same transaction, staker A cancels the new minipool so his AVAX cannot get staked and he has no risk of losing funds

Staker A can do this for any Minipool that becomes Withdrawable.

Tools Used

VSCode

Recommended Mitigation Steps

As outlined above, the Minipool that is overwritten should only be overwritten by the owner and must not be in the Withdrawable state.

Make the following changes to the MinipoolManager.createMinipool function:

                if (minipoolIndex != -1) {
+                       onlyOwner(minipoolIndex);
+                       bytes32 statusKey = keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status"));
+                       MinipoolStatus currentStatus = MinipoolStatus(getUint(statusKey));
+                       require(currentStatus != MinipoolStatus.Withdrawable, "Rewards not withdrawn");
                        requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
                        resetMinipoolData(minipoolIndex);

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L385-L440 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#L670-L683 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L94-L97

Vulnerability details

Impact

When staking is done, a Rialto multisig calls MinipoolManager.recordStakingEnd (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L385-L440).

If the avaxTotalRewardAmt has the value zero, the MinipoolManager will slash the node operator's GGP.

The issue is that the amount to slash can be greater than the GGP balance the node operator has staked.

This will cause the call to MinipoolManager.recordStakingEnd to revert because an underflow is detected.

This means a node operator can create a minipool that cannot be slashed.

A node operator must provide at least 10% of avaxAssigned as collateral by staking GGP.

It is assumed that a node operator earns AVAX at a rate of 10% per year.

So if a Minipool is created with a duration of > 365 days, the 10% collateral is not sufficient to pay the expected rewards.

This causes the function call to revert.

Another cause of the revert can be that the GGP price in AVAX changes. Specifically if the GGP price falls, there needs to be slashed more GGP.

Therefore if the GGP price drops enough it can cause the call to slash to revert.

I think it is important to say that with any collateralization ratio this can happen. The price of GGP must just drop enough or one must use a long enough duration.

The exact impact of this also depends on how the Rialto multisig handles failed calls to MinipoolManager.recordStakingEnd.

It looks like if this happens, MinipoolManager.recordStakingError (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L484-L515) is called.

This allows the node operator to withdraw his GGP stake.

So in summary a node operator can create a Minipool that cannot be slashed and probably remove his GGP stake when it should have been slashed.

Proof of Concept

When calling MinipoolManager.recordStakingEnd (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L385-L440) and the avaxTotalRewardAmt parameter is zero, the node operator is slashed:

// No rewards means validation period failed, must slash node ops GGP.
if (avaxTotalRewardAmt == 0) {
    slash(minipoolIndex);
}

The MinipoolManager.slash function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L670-L683) then calculates expectedAVAXRewardsAmt and from this slashGGPAmt:

uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt")));
uint256 expectedAVAXRewardsAmt = getExpectedAVAXRewardsAmt(duration, avaxLiquidStakerAmt);
uint256 slashGGPAmt = calculateGGPSlashAmt(expectedAVAXRewardsAmt);

Downstraem there is then a revert due to underflow because of the following line in Staking.decreaseGGPStake (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L94-L97):

subUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpStaked")), amount);

You can add the following foundry test to MinipoolManager.t.sol:

function testRecordStakingEndWithSlashFail() public {
    uint256 duration = 366 days;
    uint256 depositAmt = 1000 ether;
    uint256 avaxAssignmentRequest = 1000 ether;
    uint256 validationAmt = depositAmt + avaxAssignmentRequest;
    uint128 ggpStakeAmt = 100 ether;

    vm.startPrank(nodeOp);
    ggp.approve(address(staking), MAX_AMT);
    staking.stakeGGP(ggpStakeAmt);
    MinipoolManager.Minipool memory mp1 = createMinipool(depositAmt, avaxAssignmentRequest, duration);
    vm.stopPrank();

    address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
    vm.prank(liqStaker1);
    ggAVAX.depositAVAX{value: MAX_AMT}();

    vm.prank(address(rialto));
    minipoolMgr.claimAndInitiateStaking(mp1.nodeID);

    bytes32 txID = keccak256("txid");
    vm.prank(address(rialto));
    minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);

    vm.startPrank(address(rialto));

    skip(duration);

    minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);
}

See that it runs successfully with duration = 365 days and fails with duration = 366 days.

The similar issue occurs when the GGP price drops. I chose to implement the test with duration as the cause for the underflow because your tests use a fixed AVAX/GGP price.

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

You should check if the amount to be slashed is greater than the node operator's GGP balance. If this is the case, the amount to be slashed should be set to the node operator's GGP balance.

I believe this check can be implemented within the MinipoolManager.slash function without breaking any of the existing accounting logic.

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"));

    if (slashGGPAmt > staking.getGGPStake(owner)) {
        slashGGPAmt = staking.getGGPStake(owner);
    }
    
    staking.slashGGP(owner, slashGGPAmt);
}

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L209-L216

Vulnerability details

Impact

The ProtocolDAO.upgradeExistingContract function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L209-L216) allows to upgrade an existing contract in the protocol. E.g. it can be used to upgrade the Staking contract.

The function takes newAddr, newName and existingAddr as arguments.

It only really makes sense to upgrade an existing contract to a new contract that has the same name as the old contract. E.g. the staking contract must be called Staking before and after the upgrade. This is because other contracts look up the contract address by the contract's name:

Staking(getContractAddress("Staking"))

However the ProtocolDAO.upgradeExistingContract function calls registerContract first and then unregisterContract. Because both the new and the old contract have the same name, the contract.address entry of the new contract gets deleted again when unregisterContract is called.

Proof of Concept

1. ProtocolDAO.upgradeExistingContract is called to upgrade the Staking contract with these arguments: newAddr=0x02, newName="Staking", existingAddr=0x01.
2. The call to registerContract works fine
3. However after that, unregisterContract(0x01) is called which executes this line: deleteAddress(keccak256(abi.encodePacked("contract.address", name)));. This deletes the contract.address entry for the Staking contract. So other contracts in the protocol cannot find the Staking contract which makes the protocol inoperable

Tools Used

VSCode

Recommended Mitigation Steps

Call unregisterContract first and then registerContract such that the ProtocolDAO.upgradeExistingContract looks like this:

function upgradeExistingContract(
    address newAddr,
    string memory newName,
    address existingAddr
) external onlyGuardian {
    unregisterContract(existingAddr);
    registerContract(newAddr, newName);
}

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

L-07 It should be possible to assign Minipool to a new Multisig MinipoolManager.sol 1

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

L-04 Staking.restakeGGP function should have whenNotPaused modifier Staking.sol 1

Lines of code

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

Vulnerability details

Impact

The RewardsPool.inflate function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L82-L100) is used to inflate the circulating GGP supply by a fixed rate (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ProtocolDAO.sol#L41) every InflationIntervalSeconds.

The RewardsPool.getInflationIntervalsElapsed function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L54-L61) is used to calculate how many intervals have passed since inflation was last applied.

The issue is that the InflationIntervalStartTime variable is incremented by inflationIntervalElapsedSeconds (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L98).

However inflation is not applied to all of these passed seconds because the amount of inflation intervals is rounded down.

In effect this causes the actual inflation rate to be lower than what is specified by the inflation rate variable.

So the GGP rewards are lower than they should be.

Proof of Concept

Let's assume the following values:

• InflationIntervalStartTime = 10000
• block.timestamp = 10199
• InflationInvervalSeconds = 100

Now inflation is applied to (10199 - 10000) / 100 = 1 interval.
The InflationIntervalStartTime is then incremented by 10199 - 10000 = 199.

However inflation is only applied to 100 seconds. So the InflationIntervalStartTime should be incremented by only 100.

Tools Used

VSCode

Recommended Mitigation Steps

The calculation of inflationIntervalElapsedSeconds (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L84) in the RewardsPool.inflate function should be changed like this:

        function inflate() internal {
                ProtocolDAO dao = ProtocolDAO(getContractAddress("ProtocolDAO"));
-               uint256 inflationIntervalElapsedSeconds = (block.timestamp - getInflationIntervalStartTime());
+               uint256 inflationIntervalElapsedSeconds = getInflationIntervalsElapsed() * dao.getInflationIntervalSeconds();
                (uint256 currentTotalSupply, uint256 newTotalSupply) = getInflationAmt();
 
                TokenGGP ggp = TokenGGP(getContractAddress("TokenGGP"));

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L444-L478 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L152-L175 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L287-L303 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L298 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L457

Vulnerability details

Impact

The MinipoolManager.recreateMinipool function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L444-L478) moves the Minipool into the Prelaunch state.

The Prelaunch state can be entered from the Withdrawable as well as the Finished state (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L152-L175).

The MinipoolManager.recreateMinipool function is intended to be called from the Withdrawable state, i.e. before withdrawal has taken place.

However since the Prelaunch state can be entered from the Finished state, MinipoolManager.withdrawMinipoolFunds (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L287-L303), which puts the Minipool into the Finished state, can be called before MinipoolManager.recreateMinipool.

So a malicious staker can front-run the Rialto multisig's transaction to recreate the minipool and withdraw his funds before the Minipool is recreated.

This is an issue because the funds can then be withdrawn a second time when the recreated Minipool is in the Withdrawable state again.

Proof of Concept

The actual exploitation of this issue is not as easy as it might seem.

Consider this:

• A malicious staker creates a Minipool with 1000 AVAX stake. The Minipool earns a reward of 10 AVAX
• The staker front-runs the Rialto multisig and withdraws his funds before the Minipool is recreated. This means that in MinipoolManager.withdrawMinipoolFunds his AVAX stake is set to 0 (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L298)
• The call to MinipoolManager.recreateMinipool only increases the AVAX stake by 10 AVAX (i.e. the reward amount) (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L457)
• So when the Minipool is in the Withdrawable state again, the withdrawal fails because reducing the AVAX stake reverts due to underflow.
• The trick is that the stake must create a new minipool with 1000 AVAX stake. Now his AVAX stake is big enough to withdraw the funds of the original pool (i.e. the 1010 AVAX base amount + rewards)
• Alltogether the staker has withdrawn 1020 AVAX (1010 AVAX - 1000 AVAX + 1010 AVAX + rewards second staking period) plus the rewards from the second staking period
• You can see that he has withdrawn 10 more AVAX than he should have

In conclusion this exploit allows the staker to withdraw staking rewards multiple times. The staker must keep track of his staked AVAX amount such that all operations succeed though.

Withdrawing rewards multiple times is possible since MinipoolManager.recreateMinipool increases the AVAX stake by the rewards amount even though the rewards were withdrawn already.

Tools Used

VSCode

Recommended Mitigation Steps

The MinipoolManager.recreateMinpool function should check that the Minipool status is not Finished.
The status can only become Finished when either withdrawal has taken place or MinpoolManager.finishFailedMinipoolByMultisig was called.
In both cases recreating the Minipool should not be possible.

So change the MinipoolManager.recreateMinipool function like this:

        function recreateMinipool(address nodeID) external whenNotPaused {
                int256 minipoolIndex = onlyValidMultisig(nodeID);
+               bytes32 statusKey = keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status"));
+               MinipoolStatus currentStatus = MinipoolStatus(getUint(statusKey));
+               require(currentStatus != MinipoolStatus.Finished, "State canot be Finished");
                requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
                Minipool memory mp = getMinipool(minipoolIndex);

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L385-L440 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#L670-L683 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L94-L97

Vulnerability details

Impact

When staking is done, a Rialto multisig calls MinipoolManager.recordStakingEnd (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L385-L440).

If the avaxTotalRewardAmt has the value zero, the MinipoolManager will slash the node operator's GGP.

The issue is that the amount to slash can be greater than the GGP balance the node operator has staked.

This will cause the call to MinipoolManager.recordStakingEnd to revert because an underflow is detected.

This means a node operator can create a minipool that cannot be slashed.

A node operator must provide at least 10% of avaxAssigned as collateral by staking GGP.

It is assumed that a node operator earns AVAX at a rate of 10% per year.

So if a Minipool is created with a duration of > 365 days, the 10% collateral is not sufficient to pay the expected rewards.

This causes the function call to revert.

Another cause of the revert can be that the GGP price in AVAX changes. Specifically if the GGP price falls, there needs to be slashed more GGP.

Therefore if the GGP price drops enough it can cause the call to slash to revert.

I think it is important to say that with any collateralization ratio this can happen. The price of GGP must just drop enough or one must use a long enough duration.

The exact impact of this also depends on how the Rialto multisig handles failed calls to MinipoolManager.recordStakingEnd.

It looks like if this happens, MinipoolManager.recordStakingError (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L484-L515) is called.

This allows the node operator to withdraw his GGP stake.

So in summary a node operator can create a Minipool that cannot be slashed and probably remove his GGP stake when it should have been slashed.

Proof of Concept

When calling MinipoolManager.recordStakingEnd (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L385-L440) and the avaxTotalRewardAmt parameter is zero, the node operator is slashed:

// No rewards means validation period failed, must slash node ops GGP.
if (avaxTotalRewardAmt == 0) {
    slash(minipoolIndex);
}

The MinipoolManager.slash function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L670-L683) then calculates expectedAVAXRewardsAmt and from this slashGGPAmt:

uint256 avaxLiquidStakerAmt = getUint(keccak256(abi.encodePacked("minipool.item", index, ".avaxLiquidStakerAmt")));
uint256 expectedAVAXRewardsAmt = getExpectedAVAXRewardsAmt(duration, avaxLiquidStakerAmt);
uint256 slashGGPAmt = calculateGGPSlashAmt(expectedAVAXRewardsAmt);

Downstraem there is then a revert due to underflow because of the following line in Staking.decreaseGGPStake (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L94-L97):

subUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".ggpStaked")), amount);

You can add the following foundry test to MinipoolManager.t.sol:

function testRecordStakingEndWithSlashFail() public {
    uint256 duration = 366 days;
    uint256 depositAmt = 1000 ether;
    uint256 avaxAssignmentRequest = 1000 ether;
    uint256 validationAmt = depositAmt + avaxAssignmentRequest;
    uint128 ggpStakeAmt = 100 ether;

    vm.startPrank(nodeOp);
    ggp.approve(address(staking), MAX_AMT);
    staking.stakeGGP(ggpStakeAmt);
    MinipoolManager.Minipool memory mp1 = createMinipool(depositAmt, avaxAssignmentRequest, duration);
    vm.stopPrank();

    address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
    vm.prank(liqStaker1);
    ggAVAX.depositAVAX{value: MAX_AMT}();

    vm.prank(address(rialto));
    minipoolMgr.claimAndInitiateStaking(mp1.nodeID);

    bytes32 txID = keccak256("txid");
    vm.prank(address(rialto));
    minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);

    vm.startPrank(address(rialto));

    skip(duration);

    minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);
}

See that it runs successfully with duration = 365 days and fails with duration = 366 days.

The similar issue occurs when the GGP price drops. I chose to implement the test with duration as the cause for the underflow because your tests use a fixed AVAX/GGP price.

Tools Used

VSCode, Foundry

Recommended Mitigation Steps

You should check if the amount to be slashed is greater than the node operator's GGP balance. If this is the case, the amount to be slashed should be set to the node operator's GGP balance.

I believe this check can be implemented within the MinipoolManager.slash function without breaking any of the existing accounting logic.

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"));

    if (slashGGPAmt > staking.getGGPStake(owner)) {
        slashGGPAmt = staking.getGGPStake(owner);
    }
    
    staking.slashGGP(owner, slashGGPAmt);
}

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L88-L109

Vulnerability details

Impact

The TokenggAVAX contract is supposed to stream rewards in AVAX to ggAVAX holders over a period of 14 days.

The 14 days period however is only an upper limit for the reward streaming period.

The way the reward streaming period is calculated can make is as small as 1 second (due to rounding).

This is clearly not intended and you should calculate nextRewardsCycleEnd differently such that the 14 day period is ensured.

Proof of Concept

Rewards are streamed during a period of length rewardsCycleEnd - lastSync in the totalAssets function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L113-L130).

rewardsCycleEnd and lastSync are calculated in the syncRewards function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L88-L109).

I will show with a simple example that rewardsCycleEnd - lastSync can be as small as 1 second meaning that all rewards that are supposed to be streamed over a 14 days period are streamed within 1 second:

syncRewards() is called with:

block.timestamp = 10099,
rewardsCycleLength = 100

nextRewardsCycleEnd is calculated:

uint32 nextRewardsCycleEnd = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;

timestamp + rewardsCycleLength is equal to 10199. Dividing this by rewardsCycleLength results in 101 due to rounding.
101 * 100 is equal to 10100.

So nextRewardsCycleEnd = 10100 and lastSync = 10099.

This means that after 1 second all rewards are streamed.

Tools Used

VSCode

Recommended Mitigation Steps

In the code it is stated that you calculate uint32 nextRewardsCycleEnd = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength; because nextRewardsCycleEnd should be divisible by rewardsCycleLength.

I see no reason for this requirement and instead propose to calculate nextRewardsCycleEnd like this:

uint32 nextRewardsCycleEnd = (timestamp + rewardsCycleLength);

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/ClaimNodeOp.sol#L81-L84 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L51

Vulnerability details

Impact

The MinipoolManager.recordStakingError function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L484-L515) does not decrease the minipoolCount of the staker.

This means that if a staker has a minipool that encounters an error, his minipoolCount can never go to zero again.

This is bad because the minipoolCount is used in ClaimNodeOp.calculateAndDistributeRewards to determine if the rewardsStartTime of the staker should be reset (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L81-L84).

Since the minipoolCount cannot go to zero, the rewardsStartTime will never be reset.

This means that the staker is immediately eligible for rewards when he creates a minipool again whereas he should have to wait rewardsEligibilityMinSeconds before he is eligible (which is 14 days at the moment) (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L51).

To conclude, failing to decrease the minipoolCount allows the staker to earn higher rewards because he is eligible for staking right after he creates a new minipool and does not have to wait again.

Proof of Concept

I have created the following test that you can add to the MinipoolManager.t.sol file that logs the minipoolCount in the Staking, Error and Finished state.

The minipoolCount is always 1 although it should decrease to 0 when recordStakingError is called.

function testRecordStakingErrorWrongMinipoolCount() public {
    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();

    address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
    vm.prank(liqStaker1);
    ggAVAX.depositAVAX{value: MAX_AMT}();

    vm.prank(address(rialto));
    minipoolMgr.claimAndInitiateStaking(mp1.nodeID);

    bytes32 txID = keccak256("txid");
    vm.prank(address(rialto));
    minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);

    bytes32 errorCode = "INVALID_NODEID";

    int256 minipoolIndex = minipoolMgr.getIndexOf(mp1.nodeID);

    vm.prank(nodeOp);
    // minipool count when in "Staking" state: 1
    console.log(staking.getMinipoolCount(nodeOp));
    vm.prank(address(rialto));
    minipoolMgr.recordStakingError{value: validationAmt}(mp1.nodeID, errorCode);
    vm.prank(nodeOp);
    // minipool count when in "Error" state: 1
    console.log(staking.getMinipoolCount(nodeOp));

    vm.prank(address(rialto));

    assertEq(vault.balanceOf("MinipoolManager"), depositAmt);

    MinipoolManager.Minipool memory mp1Updated = minipoolMgr.getMinipool(minipoolIndex);

    vm.prank(address(rialto));
    minipoolMgr.finishFailedMinipoolByMultisig(mp1Updated.nodeID);
    MinipoolManager.Minipool memory mp1finished = minipoolMgr.getMinipool(minipoolIndex);
    vm.prank(nodeOp);
    // minipool count when in "Finished" state: 1
    console.log(staking.getMinipoolCount(nodeOp));
}

Tools Used

VSCode

Recommended Mitigation Steps

You need to simply add the line staking.decreaseMinipoolCount(owner); to the MinipoolManager.recordStakingError function.

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L156-L162

Vulnerability details

Impact

The TokenggAVAX contract (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L24) can be paused.

The whenTokenNotPaused modifier is applied to the following functions (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L225-L239):
previewDeposit, previewMint, previewWithdraw and previewRedeem

Thereby any calls to functions that deposit or withdraw funds revert.

There are two functions (maxWithdraw and maxRedeem) that calculate the max amount that can be withdrawn or redeemed respectively.

Both functions return 0 if the TokenggAVAX contract is paused.

The issue is that TokenggAVAX does not override the maxDeposit and maxMint functions (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L156-L162) in the ERC4626Upgradable contract like it does for maxWithdraw and maxRedeem.

Thereby these two functions return a value that cannot actually be deposited or minted.

This can cause any components that rely on any of these functions to return a correct value to malfunction.

So maxDeposit and maxMint should return the value 0 when TokenggAVAX is paused.

Proof of Concept

1. The TokenggAVAX contract is paused by calling Ocyticus.pauseEverything (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L37-L43)
2. TokenggAVAX.maxDeposit returns type(uint256).max (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L157)
3. However deposit cannot be called with this value because it is paused (previewDeposit reverts because of the whenTokenNotPaused modifier) (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/upgradeable/ERC4626Upgradeable.sol#L44)

Tools Used

VSCode

Recommended Mitigation Steps

The maxDeposit and maxMint functions should be overridden by TokenggAVAX just like maxWithdraw and maxRedeem are overridden and return 0 when the contract is paused (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L206-L223).

So add these two functions to the TokenggAVAX contract:

function maxDeposit(address) public view override returns (uint256) {
    if (getBool(keccak256(abi.encodePacked("contract.paused", "TokenggAVAX")))) {
        return 0;
    }
    return return type(uint256).max;
}

function maxMint(address) public view override returns (uint256) {
    if (getBool(keccak256(abi.encodePacked("contract.paused", "TokenggAVAX")))) {
        return 0;
    }
    return return type(uint256).max;
}

GoGoPool Low Risk and Non-Critical Issues

Summary

[L-00] Contracts inherit from BaseAbstract but do not set version variable

The BaseAbstract contract has a version variable that should be set in the child contracts. Some child contracts set version to 1. Others fail to set the version variable which means it is set to 0.

This can cause issues in any components integrating with the GoGoPool protocol that check the version of the contracts.

These are the contracts that do not but should set the version variable:

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

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L24

[L-01] Function should emit event

Functions that make important changes to the protocol should emit events such that other components can listen for them on the blockchain.

This allows for more robust monitoring of the application.

Ocyticus.sol

There are 5 functions that should emit an event.

addDefender:
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L27-L29

removeDefender:
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L32-L34

pauseEverything:
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L37-L43

resumeEverything:
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L47-L52

disableAllMultisigs:
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L55-L67

MinipoolManager.sol

The withdrawMinipoolFunds function changes the status of a minipool.
So it should emit the MinipoolStatusChangedEvent like the other functions that change the minipool status.

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

ClaimNodeOp.sol

There is 1 function that should emit an event.

calculateAndDistributeRewards:
https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/ClaimNodeOp.sol#L56-L85

[L-02] It should be possible to pause / unpause contracts individually

The current functionality to pause contracts is located in Ocyticus.sol.
There you can see that it is only possible to pause all pausable contracts at once using the pauseEverything function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Ocyticus.sol#L37-L43).

It should be possible to pause contracts individually.
Specifically the TokenggAVAX contract can in some cases still be unpaused while MinipoolManager and Staking are paused such that users can still redeem their shares.

Similarly it should be possible to unpause contracts individually.

[L-03] NewRewardsCycleStarted event emits wrong amount

The RewardsPool.startRewardsCycle function emits a NewRewardsCycleStarted event (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/RewardsPool.sol#L161).
However the event uses the RewardsCycleTotalAmount before it is updated in the inflate function.

The updated amount is the amount that is actually distributed as reward so it should be the amount that is emitted as well.

So emitting the event should take place after inflate is called.

[L-04] Staking.restakeGGP function should have whenNotPaused modifier

When the Staking contract is paused, Staking.stakeGGP and Staking.withdrawGGP cannot be called because they have the whenNotPaused modifier:

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

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

The issue is that the Staking.restakeGGP function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L328) does not have this modifier which allows stakers to restake their GGP rewards using the ClaimNodeOp.claimAndRestake function when the Staking contract is paused.

This behavior is inconsistent because pausing the Staking contract does not actually ensure that the staked GGP amounts do not change.

So you should add the whenNotPaused modifier to the Staking.restakeGGP function.

This ensures that by calling ClaimNodeOp.claimAndRestake, stakers can only withdraw their rewards but not restake them.

[L-05] Staking.getStaker function does not set avaxAssignedHighWater value

The Staking.getStaker function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L405-L414) returns all information about a staker.

However it does not set the avaxAssignedHighWater variable of the Staker struct.
This means that staker.avaxAssignedHighWater will always equal zero which can be the wrong value, leading to errors in any components that rely on this value.

You should add the following line to the Staking.getStaker function:

staker.avaxAssignedHighWater = getUint(keccak256(abi.encodePacked("staker.item", stakerIndex, ".avaxAssignedHighWater")));

[L-06] Function access modifier can be more restrictive

The Staking.increaseAVAXAssignedHighWater and Staking.resetAVAXAssignedHighWater functions currently make use of the onlyRegisteredNetworkContract modifier:

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

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

Both functions are only called in one other contract so they can use a more restrictive access modifier.

Staking.increaseAVAXAssignedHighWater should use onlySpecificRegisteredContract("MinipoolManager", msg.sender).

Staking.resetAVAXAssignedHighWater should use onlySpecificRegisteredContract("ClaimNodeOp", msg.sender).

[L-07] It should be possible to assign Minipool to a new Multisig

When a Minipool is created, a Multisig is assigned to it (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MinipoolManager.sol#L236).
However there is no possibility to assign a different Multisig to an existing Minipool.

There can be scenarios that make it necessary to assign a new Multisig to a Minipool. E.g. if something changes about Avalanche Staking such that the old Multisig is not compatible anymore.

Therefore it should be possible to change Multisigs. E.g. allow the guardian to change Multisigs.

[N-00] amountAvailableForStaking should return 0 instead of revert due to underflow

The amountAvailableForStaking function (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L132-L140) in TokenggAVAX.sol calculates:

return totalAssets_ - reservedAssets - stakingTotalAssets;

This calculation can revert due to underflow because users can withdraw so much funds that stakingTotalAssets is equal to totalAssets_. So if reservedAssets is subtracted as well this causes an underflow.

You should add this check above the line that can underflow:

if (totalAssets_ < reservedAssets + stakingTotalAssets) {
    return 0;
}

It looks like your existing code does not expect the amountAvailableForStaking function to revert and actually expects it to return 0 instead.

There is no security risk when the code reverts due to underflow but making it return 0 makes the code cleaner.

[N-01] Remove TODO comments

TODO comments left over from development should not stay in production code.

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/BaseAbstract.sol#L6-L8

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

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

[N-02] Remove unnecessary imports

Imports that are not actually used should be removed to make the code cleaner and accurately show the dependencies.

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

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

[N-03] "Data Storage Schema" comment is missing entries

In the Staking.sol file there is a "Data Storage Schema" comment that explains the storage schema of a "staker" (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/Staking.sol#L15-L27).

However there are missing entries (minipoolCount, rewardsStartTime, ggpRewards, lastRewardsCycleCompleted).

Add these entries to complete the storage schema.

[N-04] Use require instead of assert

The receive function in TokenggAVAX (https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/tokens/TokenggAVAX.sol#L82-L84) uses assert to make sure that the msg.sender is the WAVAX token.

The reason you should use require instead of assert is that assert consumes all remaining Gas if the condition is not met, whereas require returns the remaining Gas to the caller.

By using require, if someone sends AVAX mistakenly, his remaining Gas will be returned.

[N-05] Use requireValidStaker instead of getIndexOf

There are 6 instances where getIndexOf is used and should be replaced.
Instead you should use requireValidStaker which reverts if the staker address is invalid.

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

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

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

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

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

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