Y2k Finance contest - rbserver's results

Lines of code

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/Vault.sol#L350-L352 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L203-L234 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L378-L426

Vulnerability details

Impact

For a market, if users only deposit in the hedge vault or only deposit in the risk vault but not in both, then these users will lose their deposits and receive nothing when they call the following withdraw function after the depeg event occurs.

If the vault that has deposits is called Vault A, and the counterparty vault that has no deposit is called Vault B, then:

• As shown by the triggerDepeg function below, when executing insrVault.sendTokens(epochEnd, address(riskVault)) and riskVault.sendTokens(epochEnd, address(insrVault)), the deposits of Vault A are transferred to Vault B but nothing is transferred to Vault A since Vault B has no deposit;
• When triggerDepeg executes insrVault.setClaimTVL(epochEnd, riskVault.idFinalTVL(epochEnd)) and riskVault.setClaimTVL(epochEnd, insrVault.idFinalTVL(epochEnd)), Vault B's idClaimTVL[id] is set to Vault A's idFinalTVL(epochEnd)) but Vault A's idClaimTVL[id] is set to 0 because Vault B's idFinalTVL(epochEnd) is 0.

Because of these, calling the beforeWithdraw function below will return a 0 entitledAmount, and calling withdraw then transfers that 0 amount to the user who has deposited. As a result, these users' deposits are transferred to the counterparty vault, and they receive nothing at all.

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)
    {
        address[] memory vaultsAddress = vaultFactory.getVaults(marketIndex);
        Vault insrVault = Vault(vaultsAddress[0]);
        Vault riskVault = Vault(vaultsAddress[1]);

        //require this function cannot be called twice in the same epoch for the same vault
        if(insrVault.idFinalTVL(epochEnd) != 0)
            revert NotZeroTVL();
        if(riskVault.idFinalTVL(epochEnd) != 0) 
            revert NotZeroTVL();

        insrVault.endEpoch(epochEnd, true);
        riskVault.endEpoch(epochEnd, true);

        insrVault.setClaimTVL(epochEnd, riskVault.idFinalTVL(epochEnd));
        riskVault.setClaimTVL(epochEnd, insrVault.idFinalTVL(epochEnd));

        insrVault.sendTokens(epochEnd, address(riskVault));
        riskVault.sendTokens(epochEnd, address(insrVault));

        VaultTVL memory tvl = VaultTVL(
            riskVault.idClaimTVL(epochEnd),
            insrVault.idClaimTVL(epochEnd),
            riskVault.idFinalTVL(epochEnd),
            insrVault.idFinalTVL(epochEnd)
        );

        emit DepegInsurance(
            keccak256(
                abi.encodePacked(
                    marketIndex,
                    insrVault.idEpochBegin(epochEnd),
                    epochEnd
                )
            ),
            tvl,
            true,
            epochEnd,
            block.timestamp,
            getLatestPrice(insrVault.tokenInsured())
        );
    }

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L350-L352

    function setClaimTVL(uint256 id, uint256 claimTVL) public onlyController {
        idClaimTVL[id] = claimTVL;
    }

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L203-L234

    function withdraw(
        uint256 id,
        uint256 assets,
        address receiver,
        address owner
    )
        external
        override
        epochHasEnded(id)
        marketExists(id)
        returns (uint256 shares)
    {
        if(
            msg.sender != owner &&
            isApprovedForAll(owner, receiver) == false)
            revert OwnerDidNotAuthorize(msg.sender, owner);

        shares = previewWithdraw(id, assets); // No need to check for rounding error, previewWithdraw rounds up.

        uint256 entitledShares = beforeWithdraw(id, shares);
        _burn(owner, id, shares);

        //Taking fee from the amount
        uint256 feeValue = calculateWithdrawalFeeValue(entitledShares, id);
        entitledShares = entitledShares - feeValue;
        asset.transfer(treasury, feeValue);

        emit Withdraw(msg.sender, receiver, owner, id, assets, entitledShares);
        asset.transfer(receiver, entitledShares);

        return entitledShares;
    }

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L378-L426

    function beforeWithdraw(uint256 id, uint256 amount)
        public
        view
        returns (uint256 entitledAmount)
    {
        // in case the risk wins aka no depeg event
        // risk users can withdraw the hedge (that is paid by the hedge buyers) and risk; withdraw = (risk + hedge)
        // hedge pay for each hedge seller = ( risk / tvl before the hedge payouts ) * tvl in hedge pool
        // in case there is a depeg event, the risk users can only withdraw the hedge
        if (
            keccak256(abi.encodePacked(symbol)) ==
            keccak256(abi.encodePacked("rY2K"))
        ) {
            if (!idDepegged[id]) {
                //depeg event did not happen
                /*
                entitledAmount =
                    (amount / idFinalTVL[id]) *
                    idClaimTVL[id] +
                    amount;
                */
                entitledAmount =
                    amount.divWadDown(idFinalTVL[id]).mulDivDown(
                        idClaimTVL[id],
                        1 ether
                    ) +
                    amount;
            } else {
                //depeg event did happen
                entitledAmount = amount.divWadDown(idFinalTVL[id]).mulDivDown(
                    idClaimTVL[id],
                    1 ether
                );
            }
        }
        // in case the hedge wins aka depegging
        // hedge users pay the hedge to risk users anyway,
        // hedge guy can withdraw risk (that is transfered from the risk pool),
        // withdraw = % tvl that hedge buyer owns
        // otherwise hedge users cannot withdraw any Eth
        else {
            entitledAmount = amount.divWadDown(idFinalTVL[id]).mulDivDown(
                idClaimTVL[id],
                1 ether
            );
        }

        return entitledAmount;
    }

Proof of Concept

Please append the following tests in test\AssertTest.t.sol. These tests will pass to demonstrate the described scenarios.

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

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

        assertTrue(vRisk.balanceOf(chad,endEpoch) == (AMOUNT * CHAD_MULTIPLIER));
        vm.stopPrank();

        vm.warp(beginEpoch + 10 days);

        // depeg occurs
        controller.triggerDepeg(SINGLE_MARKET_INDEX, endEpoch);

        vm.startPrank(chad);

        // chad withdraws from risk vault
        uint256 assets = vRisk.balanceOf(chad,endEpoch);
        vRisk.withdraw(endEpoch, assets, chad, chad);

        assertTrue(vRisk.balanceOf(chad,endEpoch) == NULL_BALANCE);
        uint256 entitledShares = vRisk.beforeWithdraw(endEpoch, assets);
        assertTrue(entitledShares - vRisk.calculateWithdrawalFeeValue(entitledShares,endEpoch) == ERC20(WETH).balanceOf(chad));

        // chad receives nothing
        assertEq(entitledShares, 0);
        assertEq(ERC20(WETH).balanceOf(chad), 0);

        vm.stopPrank();
    }

    function testWithdrawFromHedgeAfterDepegWhenThereIsNoCounterparty() public {
        vm.deal(alice, AMOUNT);

        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, and no one deposits in risk vault
        vm.startPrank(alice);
        ERC20(WETH).approve(hedge, AMOUNT);
        vHedge.depositETH{value: AMOUNT}(endEpoch, alice);

        assertTrue(vHedge.balanceOf(alice,endEpoch) == (AMOUNT));
        vm.stopPrank();

        vm.warp(beginEpoch + 10 days);

        // depeg occurs
        controller.triggerDepeg(SINGLE_MARKET_INDEX, endEpoch);

        vm.startPrank(alice);

        // alice withdraws from hedge vault
        uint256 assets = vHedge.balanceOf(alice,endEpoch);
        vHedge.withdraw(endEpoch, assets, alice, alice);

        assertTrue(vHedge.balanceOf(alice,endEpoch) == NULL_BALANCE);
        uint256 entitledShares = vHedge.beforeWithdraw(endEpoch, assets);
        assertTrue(entitledShares - vHedge.calculateWithdrawalFeeValue(entitledShares,endEpoch) == ERC20(WETH).balanceOf(alice));
        
        // alice receives nothing
        assertEq(entitledShares, 0);
        assertEq(ERC20(WETH).balanceOf(alice), 0);

        vm.stopPrank();
    }

Tools Used

VSCode

Recommended Mitigation Steps

When users only deposit in one vault, and no one deposits in the counterparty vault, the insurance practice of hedging and risking actually does not exist. In this situation, after the epoch is started, the users, who have deposited, should be allowed to withdraw their full deposit amounts.

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.

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L378-L426 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L203-L234

Vulnerability details

Impact

In the following beforeWithdraw function, entitledAmount = amount.divWadDown(idFinalTVL[id]).mulDivDown(idClaimTVL[id], 1 ether) can be executed in several places. Because it uses division before multiplication, it is possible that entitledAmount is calculated to be 0. As the withdraw function shows below, when entitledAmount is 0, the receiver and treasury both receive 0. As a result, calling withdraw with a positive assets input can still result in transferring nothing to the receiver and treasury.

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L378-L426

    function beforeWithdraw(uint256 id, uint256 amount)
        public
        view
        returns (uint256 entitledAmount)
    {
        // in case the risk wins aka no depeg event
        // risk users can withdraw the hedge (that is paid by the hedge buyers) and risk; withdraw = (risk + hedge)
        // hedge pay for each hedge seller = ( risk / tvl before the hedge payouts ) * tvl in hedge pool
        // in case there is a depeg event, the risk users can only withdraw the hedge
        if (
            keccak256(abi.encodePacked(symbol)) ==
            keccak256(abi.encodePacked("rY2K"))
        ) {
            if (!idDepegged[id]) {
                //depeg event did not happen
                /*
                entitledAmount =
                    (amount / idFinalTVL[id]) *
                    idClaimTVL[id] +
                    amount;
                */
                entitledAmount =
                    amount.divWadDown(idFinalTVL[id]).mulDivDown(
                        idClaimTVL[id],
                        1 ether
                    ) +
                    amount;
            } else {
                //depeg event did happen
                entitledAmount = amount.divWadDown(idFinalTVL[id]).mulDivDown(
                    idClaimTVL[id],
                    1 ether
                );
            }
        }
        // in case the hedge wins aka depegging
        // hedge users pay the hedge to risk users anyway,
        // hedge guy can withdraw risk (that is transfered from the risk pool),
        // withdraw = % tvl that hedge buyer owns
        // otherwise hedge users cannot withdraw any Eth
        else {
            entitledAmount = amount.divWadDown(idFinalTVL[id]).mulDivDown(
                idClaimTVL[id],
                1 ether
            );
        }

        return entitledAmount;
    }

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L203-L234

    function withdraw(
        uint256 id,
        uint256 assets,
        address receiver,
        address owner
    )
        external
        override
        epochHasEnded(id)
        marketExists(id)
        returns (uint256 shares)
    {
        if(
            msg.sender != owner &&
            isApprovedForAll(owner, receiver) == false)
            revert OwnerDidNotAuthorize(msg.sender, owner);

        shares = previewWithdraw(id, assets); // No need to check for rounding error, previewWithdraw rounds up.

        uint256 entitledShares = beforeWithdraw(id, shares);
        _burn(owner, id, shares);

        //Taking fee from the amount
        uint256 feeValue = calculateWithdrawalFeeValue(entitledShares, id);
        entitledShares = entitledShares - feeValue;
        asset.transfer(treasury, feeValue);

        emit Withdraw(msg.sender, receiver, owner, id, assets, entitledShares);
        asset.transfer(receiver, entitledShares);

        return entitledShares;
    }

Proof of Concept

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

    function testReceiveZeroDueToDivBeingPerformedBeforeMul() public {
        vm.deal(alice, 1e24);
        vm.deal(chad, 1e24);

        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 1e24 in hedge vault
        vm.startPrank(alice);
        ERC20(WETH).approve(hedge, 1e24);
        vHedge.depositETH{value: 1e24}(endEpoch, alice);
        vm.stopPrank();

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

        vm.warp(beginEpoch + 10 days);

        // depeg occurs
        controller.triggerDepeg(SINGLE_MARKET_INDEX, endEpoch);

        vm.startPrank(chad);

        // chad withdraws 1e5 from risk vault
        vRisk.withdraw(endEpoch, 1e5, chad, chad);

        // the amount to chad is 0 because division is performed before multiplication
        uint256 entitledShares = vRisk.beforeWithdraw(endEpoch, 1e5);

        // chad receives nothing
        assertEq(entitledShares, 0);
        assertEq(ERC20(WETH).balanceOf(chad), 0);

        // the amount to chad would be positive when multiplication is performed before division
        uint256 entitledShares2 = (1e5 * vRisk.idClaimTVL(endEpoch)) / vRisk.idFinalTVL(endEpoch);
        assertTrue(entitledShares2 > entitledShares);

        vm.stopPrank();
    }

Tools Used

VSCode

Recommended Mitigation Steps

entitledAmount = amount.divWadDown(idFinalTVL[id]).mulDivDown(idClaimTVL[id], 1 ether) in the beforeWithdraw function can be updated to the following code.

    entitledAmount = (amount * idClaimTVL[id]) / idFinalTVL[id]

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L87-L91 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L152-L174 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/VaultFactory.sol#L327-L338 https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L287-L289

Vulnerability details

Impact

As shown by the following epochHasNotStarted modifier, which is used by the deposit function below, users can only deposit when block.timestamp <= idEpochBegin[id] - timewindow holds true. Before depositing, a user can check if this relationship is true at that moment; if so, she or he can call the deposit function. However, just before the user's deposit function call is executed, the admin unexpectedly calls the VaultFactory.changeTimewindow function below, which further calls the Vault.changeTimewindow function below, to increase the timewindow. Since the admin's VaultFactory.changeTimewindow transaction is executed before the user's deposit transaction and the timewindow change takes effect immediately, it is possible that the user's deposit function call will revert. Besides wasting gas, the user can feel confused and unfair because her or his deposit transaction should be executed successfully if VaultFactory.changeTimewindow is not called unexpectedly.

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L87-L91

    modifier epochHasNotStarted(uint256 id) {
        if(block.timestamp > idEpochBegin[id] - timewindow)
            revert EpochAlreadyStarted();
        _;
    }

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L152-L174

    function deposit(
        uint256 id,
        uint256 assets,
        address receiver
    )
        public
        override
        marketExists(id)
        epochHasNotStarted(id)
        nonReentrant
        returns (uint256 shares)
    {
        // Check for rounding error since we round down in previewDeposit.
        require((shares = previewDeposit(id, assets)) != 0, "ZeroValue");

        asset.transferFrom(msg.sender, address(this), shares);

        _mint(receiver, id, shares, EMPTY);

        emit Deposit(msg.sender, receiver, id, shares, shares);

        return shares;
    }

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/VaultFactory.sol#L327-L338

    function changeTimewindow(uint256 _marketIndex, uint256 _timewindow)
        public
        onlyAdmin
    {
        address[] memory vaults = indexVaults[_marketIndex];
        Vault insr = Vault(vaults[0]);
        Vault risk = Vault(vaults[1]);
        insr.changeTimewindow(_timewindow);
        risk.changeTimewindow(_timewindow);

        emit changedTimeWindow(_marketIndex, _timewindow);
    }

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L287-L289

    function changeTimewindow(uint256 _timewindow) public onlyFactory {
        timewindow = _timewindow;
    }

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 EpochAlreadyStarted();

    function testChangeTimeWindowUnexpectedly() 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 is able to deposit in hedge vault before the time window change
        vm.startPrank(alice);
        ERC20(WETH).approve(hedge, AMOUNT);
        vHedge.depositETH{value: AMOUNT}(endEpoch, alice);
        vm.stopPrank();

        // admin changes time window unexpectedly, which takes effect immediately
        vm.startPrank(admin);
        vaultFactory.changeTimewindow(1, 5 days);
        vm.stopPrank();

        // chad is unable to deposit in risk vault after the time window change
        vm.startPrank(chad);
        ERC20(WETH).approve(risk, AMOUNT * CHAD_MULTIPLIER);

        vm.expectRevert(EpochAlreadyStarted.selector);
        vRisk.depositETH{value: AMOUNT * CHAD_MULTIPLIER}(endEpoch, chad);
        vm.stopPrank();
    }

Tools Used

VSCode

Recommended Mitigation Steps

When calling the VaultFactory.createNewMarket or VaultFactory.deployMoreAssets function, the timewindow, which is configured for that moment, can be taken into account in the created asset's epochBegin.

Then, https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L87-L91 can be updated to the following code.

    modifier epochHasNotStarted(uint256 id) {
        if(block.timestamp > idEpochBegin[id])
            revert EpochAlreadyStarted();
        _;
    }

Lines of code

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

Vulnerability details

Impact

When calling the following triggerEndEpoch function, tvl, which is a VaultTVL type, is created as a part of the emitted DepegInsurance event after idClaimTVL and idFinalTVL are already updated for both the hedge and risk vaults. However, comparing to the fields of the VaultTVL struct definition below, insrVault.idClaimTVL(epochEnd) is incorrectly used as RISK_finalTVL and riskVault.idFinalTVL(epochEnd) is incorrectly used as INSR_claimTVL because insrVault.setClaimTVL(epochEnd, 0) has been executed, which does not occur when calling the triggerDepeg function. Because of the incorrect tvl used in the emitted DepegInsurance event, the frontend can display misleading information that confuse users, and debugging with incorrect data will be hard for developers.

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

        address[] memory vaultsAddress = vaultFactory.getVaults(marketIndex);

        Vault insrVault = Vault(vaultsAddress[0]);
        Vault riskVault = Vault(vaultsAddress[1]);

        if(insrVault.idExists(epochEnd) == false || riskVault.idExists(epochEnd) == false)
            revert EpochNotExist();

        //require this function cannot be called twice in the same epoch for the same vault
        if(insrVault.idFinalTVL(epochEnd) != 0)
            revert NotZeroTVL();
        if(riskVault.idFinalTVL(epochEnd) != 0) 
            revert NotZeroTVL();

        insrVault.endEpoch(epochEnd, false);
        riskVault.endEpoch(epochEnd, false);

        insrVault.setClaimTVL(epochEnd, 0);
        riskVault.setClaimTVL(epochEnd, insrVault.idFinalTVL(epochEnd));
        insrVault.sendTokens(epochEnd, address(riskVault));

        VaultTVL memory tvl = VaultTVL(
            riskVault.idClaimTVL(epochEnd),
            insrVault.idClaimTVL(epochEnd),
            riskVault.idFinalTVL(epochEnd),
            insrVault.idFinalTVL(epochEnd)
        );

        emit DepegInsurance(
            keccak256(
                abi.encodePacked(
                    marketIndex,
                    insrVault.idEpochBegin(epochEnd),
                    epochEnd
                )
            ),
            tvl,
            false,
            epochEnd,
            block.timestamp,
            getLatestPrice(insrVault.tokenInsured())
        );
    }

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

    struct VaultTVL {
        uint256 RISK_claimTVL;
        uint256 RISK_finalTVL;
        uint256 INSR_claimTVL;
        uint256 INSR_finalTVL;
    }

Proof of Concept

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

    function testCallingtriggerEndEpochCreatesIncorrectVaultTVL() public{
        testDeposit();

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

        Vault vHedge = Vault(hedge);
        Vault vRisk = Vault(risk);

        vm.warp(endEpoch + 1 days);

        controller.triggerEndEpoch(SINGLE_MARKET_INDEX, endEpoch);

        /* VaultTVL struct has the following structure

            struct VaultTVL {
                uint256 RISK_claimTVL;
                uint256 RISK_finalTVL;
                uint256 INSR_claimTVL;
                uint256 INSR_finalTVL;
            }

        */

        /* in controller.triggerEndEpoch, VaultTVL is created as follows after idClaimTVL and idFinalTVL for both vaults are already updated

            VaultTVL memory tvl = VaultTVL(
                riskVault.idClaimTVL(epochEnd),
                insrVault.idClaimTVL(epochEnd),
                riskVault.idFinalTVL(epochEnd),
                insrVault.idFinalTVL(epochEnd)
            );

        */

        // insrVault.idClaimTVL(epochEnd), which is vHedge.idClaimTVL(endEpoch), does not correspond to RISK_finalTVL, which should be vRisk.idFinalTVL(endEpoch)
        assertTrue(vRisk.idFinalTVL(endEpoch) != vHedge.idClaimTVL(endEpoch));

        // riskVault.idFinalTVL(epochEnd), which is vRisk.idFinalTVL(endEpoch), does not correspond to INSR_claimTVL, which should be vHedge.idClaimTVL(endEpoch) 
        assertTrue(vHedge.idClaimTVL(endEpoch) != vRisk.idFinalTVL(endEpoch));
    }

Tools Used

VSCode

Recommended Mitigation Steps

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

        VaultTVL memory tvl = VaultTVL(
            riskVault.idClaimTVL(epochEnd),
            riskVault.idFinalTVL(epochEnd),
            insrVault.idClaimTVL(epochEnd),
            insrVault.idFinalTVL(epochEnd)
        );