code-423n4 / 2022-12-gogopool-findings

1 stars 0 forks source link

Providing wrong `duration` time can lead to unexpected behavior. #412

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-gogopool/blob/2996dc7974be06800e35619101047bf1d2107c42/contracts/contract/MinipoolManager.sol#L196

Vulnerability details

Impact

The current implementation of createMinipool does not validate does the duration time is valid. If the As consequence funds of the Node Operator can be locked in the contract.

Proof of Concept

// 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 ScenariosTestHack is BaseTest {
    using FixedPointMathLib for uint256;
    uint128 internal constant ONE_K = 1_000 ether;
    uint256 internal constant TOTAL_INITIAL_GGP_SUPPLY = 22_500_000 ether;

    address private nodeOp1;
    address private nodeOp2;
    address private nodeOp3;
    address private nodeOp4;
    address private liqStaker1;
    address private liqStaker2;
    address private liqStaker3;
    address private investor1;
    address private investor2;

    function setUp() public override {
        super.setUp();

        nodeOp1 = getActorWithTokens("nodeOp1", ONE_K, ONE_K);
        nodeOp2 = getActorWithTokens("nodeOp2", ONE_K, ONE_K);
        nodeOp3 = getActorWithTokens("nodeOp3", ONE_K, ONE_K);
        nodeOp4 = getActorWithTokens("nodeOp4", ONE_K, ONE_K);
        liqStaker1 = getActorWithTokens("liqStaker1", ONE_K, 0);
        liqStaker2 = getActorWithTokens("liqStaker2", ONE_K, 0);
        liqStaker3 = getActorWithTokens("liqStaker3", ONE_K * 7, 0);
        investor1 = getInvestorWithTokens("investor1", ONE_K, ONE_K);
        investor2 = getInvestorWithTokens("investor2", ONE_K, ONE_K);

        fundGGPRewardsPool();
    }

    function fundGGPRewardsPool() public {
        // guardian is minted 100% of the supply
        vm.startPrank(guardian);
        uint256 rewardsPoolAmt = TOTAL_INITIAL_GGP_SUPPLY.mulWadDown(.20 ether);
        ggp.approve(address(vault), rewardsPoolAmt);
        vault.depositToken("RewardsPool", ggp, rewardsPoolAmt);
        vm.stopPrank();
    }

    // For this test we wont do lots of intermediate asserts, just focus on end results
    function testFullCycleHappyPath() public {
        uint256 duration = 2 weeks;
        uint256 depositAmt = dao.getMinipoolMinAVAXAssignment();
        uint256 ggpStakeAmt = depositAmt.mulWadDown(dao.getMinCollateralizationRatio());

        // Liq Stakers deposit all their AVAX and get ggAVAX in return
        vm.prank(liqStaker1);
        ggAVAX.depositAVAX{value: ONE_K}();

        vm.prank(liqStaker2);
        ggAVAX.depositAVAX{value: ONE_K}();

        vm.startPrank(nodeOp1);
        ggp.approve(address(staking), ggpStakeAmt);
        staking.stakeGGP(ggpStakeAmt);
        MinipoolManager.Minipool memory mp = createMinipool(depositAmt, depositAmt, 0);
        skip(5 seconds); //cancellation moratorium

        vm.stopPrank();
        mp = rialto.processMinipoolStart(mp.nodeID);
        skip(mp.duration);
        vm.prank(nodeOp1);
        vm.expectRevert(MinipoolManager.InvalidStateTransition.selector);
        minipoolMgr.cancelMinipool(mp.nodeID);

        int256 minipoolIndex = minipoolMgr.getIndexOf(mp.nodeID);
        store.setUint(keccak256(abi.encodePacked("minipool.item", minipoolIndex, ".status")), uint256(MinipoolStatus.Prelaunch));

        vm.startPrank(nodeOp1);
        uint256 priorBalance = nodeOp1.balance;
        vm.expectRevert(Vault.InsufficientContractBalance.selector);
        minipoolMgr.cancelMinipool(mp.nodeID);

    }
}

Tools Used

Foundry

Recommended Mitigation Steps

Implementing a validation routine is recommended to check whether the provided duration time is correct.

c4-judge commented 1 year ago

GalloDaSballo marked the issue as duplicate of #694

c4-judge commented 1 year ago

GalloDaSballo marked the issue as not a duplicate

GalloDaSballo commented 1 year ago

Am downgrading to QA because this finding only shows the lack of check for duration and doesn't go deep enough in terms of other attacks

GalloDaSballo commented 1 year ago

L

c4-judge commented 1 year ago

Duplicate of https://github.com/code-423n4/2022-12-gogopool-findings/issues/432