GoGoPool contest - danyams's results

Lines of code

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

Vulnerability details

Impact

The MinipoolManager will not be able to slash a staker's GGP depending on the minipool's duration and the GGP price in AVAX. This specifically occurs when:

staker.item<i>.ggpStaked < (expectedAVAXRewardsRate * minipool.item<i>.duration) / (365 days * ggpPriceInAVAX).

Assuming this is true, recordStakingEnd( ) will throw an arithmetic underflow error.

This vulnerability occurs due to the private function, slash( ), which is called in recordStakingEnd( ). slash( ) calls slashGGP( ) in the Staking contract, which then decreases a user's GGP stake by a given amount. However, staking.slashGGP( ) can be called in MinipoolManager with a value greater than a user's stake, causing an underflow error.

This vulnerability occurs over a wide range of scenarios since a staker's duration is not validated in createMinipool( ), ggpPriceInAVAX is variable, and a minipool owner can stake a wide range of values. For example, assuming ProtocolDAO.ExpectedAVAXRewardsRate remains at 10%, this bug will occur if the owner's stake is 10% of their assigned AVAX, the minipool's duration is 365 days, and ggpPriceInAVAX decreases by any amount.

Due to MinipoolManager's state transition logic, the only way to reset a vulnerable pool and distribute the owner's and stakers' AVAX is by calling recordStakingError( ), which does not slash the owner's staked amount.

Proof of Concept

1. Stake 10% of 1000 AVAX in GGP
2. Call createMiniPool( ) in MinipoolManager with a deposit amount of 1000 AVAX, an assignment request of 1000 AVAX, and a duration of 365 days
3. Wait 14 days and assume you earn no rewards validating
4. Assume the price of GGP in AVAX has decreased by 1/10**18
5. Have the Rialto Multisig call recordStakingEnd( )

Here is the proof of concept from the given test suite:

 --- a/test/unit/MinipoolManager.t.sol
 +++ b/test/unit/MinipoolManager.t.sol

  @@ -2,6 +2,7 @@
  pragma solidity 0.8.17;

  import "./utils/BaseTest.sol";
 +import {stdError} from "forge-std/Test.sol";
  import {FixedPointMathLib} from "@rari-capital/solmate/src/utils/FixedPointMathLib.sol";

 @@ -502,6 +503,46 @@ contract MinipoolManagerTest is BaseTest {

  +       function testRecordStakingEndWithSlashBug() public {
  +               uint256 duration = 365 days;
  +               uint256 depositAmt = 1000 ether;
  +               uint256 avaxAssignmentRequest = 1000 ether;
  +               uint256 validationAmt = depositAmt + avaxAssignmentRequest;
  +               uint128 ggpStakeAmt = 100 ether;
  +
  +               // Stake GGP  -- 1 GGP = 1 AVAX
  +               vm.startPrank(nodeOp);
  +               ggp.approve(address(staking), MAX_AMT);
  +               staking.stakeGGP(ggpStakeAmt);
  +
  +               // Create a minipool with a duration of 365 days
  +               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.startPrank(address(rialto));
  +               minipoolMgr.claimAndInitiateStaking(mp1.nodeID);
  +       
  +               bytes32 txID = keccak256("txid");
  +               minipoolMgr.recordStakingStart(mp1.nodeID, txID, block.timestamp);      
  +
  +               // Wait 14 days
  +               skip(14 days);
  +               
  +               // Decrease AVAX price by 1/10**18
  +               (uint256 _currentGGPPriceInAvax, ) = oracle.getGGPPriceInAVAX();
  +               rialto.setGGPPriceInAVAX(_currentGGPPriceInAvax - 1, block.timestamp);
  +
  +               // Have Rialto call recordStakingEnd with the param avaxTotalRewardAmt set to 0 
  +               vm.expectRevert(stdError.arithmeticError);
  +               minipoolMgr.recordStakingEnd{value: validationAmt}(mp1.nodeID, block.timestamp, 0 ether);
  +
  +               vm.stopPrank();
  +       }

Tools Used

Foundry & VSCode

Recommended Mitigation Steps

Call staking.slashGGP( ) in slash( ) only after verifying that a minipool owner's stake is greater than or equal to the expected amount of GGP to be slashed. If it is not, call slash( ) with the owner's staked GGP rather than the expected amount of slashed GGP.

@@ -673,12 +673,15 @@ contract MinipoolManager is Base, ReentrancyGuard, IWithdrawer {
                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"));
+               
+               uint256 expectedSlashGGPAmt = calculateGGPSlashAmt(expectedAVAXRewardsAmt); 
+               uint256 GGPStaked = staking.getGGPStake(owner); 
+               uint256 slashGGPAmt = (expectedSlashGGPAmt > GGPStaked) ?  GGPStaked : expectedSlashGGPAmt;
+
+               setUint(keccak256(abi.encodePacked("minipool.item", index, ".ggpSlashAmt")), slashGGPAmt);
+               emit GGPSlashed(nodeID, slashGGPAmt);
                staking.slashGGP(owner, slashGGPAmt);
        }