Ambiguous situation exists for calling triggerDepeg and triggerEndEpoch functions when block.timestamp is set to epochEnd

[code423n4]

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Controller.sol#L83-L110
https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Controller.sol#L148-L192
https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Controller.sol#L198-L248

Vulnerability details

Impact

As shown by the following isDisaster modifier, which is used by the triggerDepeg function below, and the triggerEndEpoch function below, when block.timestamp is set to epochEnd, both of the triggerDepeg and triggerEndEpoch functions are allowed to be called. This creates an ambiguous situation. If the depeg event occurs at epochEnd, the hedge users can have incentive to call the triggerDepeg function so they can gain assets from the risk users while the risk users can find this unfair because they believe that the epoch is already over. This encourages racing between the triggerDepeg and triggerEndEpoch transactions. If the triggerEndEpoch transaction is included and executed before the triggerDepeg transaction, then the risk users win and the hedge users lose while the vice versa is also true. Either way, it is advantegous to one group and is unfair to the other.

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Controller.sol#L83-L110

    modifier isDisaster(uint256 marketIndex, uint256 epochEnd) {
        ...

        if(
            block.timestamp > epochEnd
            )
            revert EpochExpired();
        _;
    }

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Controller.sol#L148-L192

    function triggerDepeg(uint256 marketIndex, uint256 epochEnd)
        public
        isDisaster(marketIndex, epochEnd)
    {
        ...
    }

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Controller.sol#L198-L248

    function triggerEndEpoch(uint256 marketIndex, uint256 epochEnd) public {
        if(
            vaultFactory.getVaults(marketIndex).length != VAULTS_LENGTH)
                revert MarketDoesNotExist(marketIndex);
        if(
            block.timestamp < epochEnd)
            revert EpochNotExpired();

        ...
    }

Proof of Concept

Please add the following error and append the following test in test\AssertTest.t.sol. This test will pass to demonstrate the described scenario.

    error NotZeroTVL();

    function testtriggerEndEpochFrontRunstriggerDepeg() public {
        vm.deal(alice, AMOUNT);
        vm.deal(chad, AMOUNT * CHAD_MULTIPLIER);

        vm.startPrank(admin);
        FakeOracle fakeOracle = new FakeOracle(oracleFRAX, STRIKE_PRICE_FAKE_ORACLE);
        vaultFactory.createNewMarket(FEE, tokenFRAX, DEPEG_AAA, beginEpoch, endEpoch, address(fakeOracle), "y2kFRAX_99*");
        vm.stopPrank();

        address hedge = vaultFactory.getVaults(1)[0];
        address risk = vaultFactory.getVaults(1)[1];
        
        Vault vHedge = Vault(hedge);
        Vault vRisk = Vault(risk);

        // alice deposits in hedge vault
        vm.startPrank(alice);
        ERC20(WETH).approve(hedge, AMOUNT);
        vHedge.depositETH{value: AMOUNT}(endEpoch, alice);
        vm.stopPrank();

        // chad deposits in risk vault
        vm.startPrank(chad);
        ERC20(WETH).approve(risk, AMOUNT * CHAD_MULTIPLIER);
        vRisk.depositETH{value: AMOUNT * CHAD_MULTIPLIER}(endEpoch, chad);
        vm.stopPrank();

        vm.warp(endEpoch);

        // when block.timestamp is set to endEpoch, triggerEndEpoch can be called to front-run triggerDepeg call
        controller.triggerEndEpoch(SINGLE_MARKET_INDEX, endEpoch);

        // after front-running, calling triggerDepeg reverts even if depeg occurs
        vm.expectRevert(NotZeroTVL.selector);
        controller.triggerDepeg(SINGLE_MARKET_INDEX, endEpoch);
    }

Tools Used

VSCode

Recommended Mitigation Steps

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Controller.sol#L105-L108 can be updated to the following code:

        if(
            block.timestamp >= epochEnd
            )
            revert EpochExpired();

or

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Controller.sol#L202-L204 can be updated to the following code:

        if(
            block.timestamp <= epochEnd)
            revert EpochNotExpired();

but not both for preventing this ambiguous situation.

[HickupHH3]

dup #69