Y2k Finance contest - unforgiven's results

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L144-L192 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L194-L248 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L331-L343 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L372-L426

Vulnerability details

Impact

Users can withdraw their funds from risk or hedge Vault after epoch ends, and functions triggerDepeg() and triggerEndEpoch() in Controller are responsible for setting the state of the Vaults and end the epochs. but when one of Vaults has 0 TVL then the logic of triggerDepeg() and triggerEndEpoch() would revert (because of transferring zero amount of asset) and that Vault's epoch would never end (endEpoch() would never get called) and users can't withdraw their funds (because beforeWithdraw() would revert becasue of division by zero) and user would lose their funds. also in this case when one of the Vaults has 0 TVL, triggerDepeg() would never get called and epoch expire even when it should be depegged.

Proof of Concept

This is triggerDepeg() and triggerEndEpoch() code:

    /** @notice Trigger depeg event
      * @param marketIndex Target market index
      * @param epochEnd End of epoch set for market
      */
    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())
        );
    }

    /** @notice Trigger epoch end without depeg event
      * @param marketIndex Target market index
      * @param epochEnd End of epoch set for market
      */
    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())
        );
    }

As you can see code calls Vault.sendTokens() to send assets of one Vault to other one, This is sendTokens() code in Vault:

    /**
    solhint-disable-next-line max-line-length
    @notice Function to be called after endEpoch and setClaimTVL functions, respecting the calls in order, after storing the TVL of the end of epoch and the TVL amount to claim, this function will allow the transfer of tokens to the counterparty vault
    @param  id uint256 in UNIX timestamp, representing the end date of the epoch. Example: Epoch ends in 30th June 2022 at 00h 00min 00sec: 1654038000
    @param _counterparty Address of the other vault, meaning address of the risk vault, if this is an hedge vault, and vice-versa
    */
    function sendTokens(uint256 id, address _counterparty)
        public
        onlyController
        marketExists(id)
    {
        asset.transfer(_counterparty, idFinalTVL[id]);
    }

As you can see it tries to transfer idFinalTVL[] amount of asset to the specified address. but when idFinalTVL[] is equal to 0 this transfer would fail and function sendTokens() would revert which would cause triggerDepeg() and triggerEndEpoch() to revert too. so the epoch state would never be set to end (Vault.endEpoch() would never get called for that epoch) and users can't withdraw their funds (when withdrawing in beforeWithdraw() code divide numbers to idClaimTVL[] and becasue epoch is not ended so idClaimTVL[] is zero and transaction would revert becasue of division by zero). So if one of the risk or hedge Vaults for specific epoch had zero deposits (0 TVL) then functions triggerDepeg() and triggerEndEpoch() in Controller can't be called for that epoch and users funds in that epoch (in one of the vaults that has deposit) would locked in contract forever and users can't withdraw them ( triggerDepeg() and triggerEndEpoch() in controller would revert becasue of trying to transfer 0 amount and withdraw() in Vault would fail becasue of division by zero). There are many scenarios that can cause one of the Vaults to have 0 deposits, for example in the case when pegged token price is already out of strike price right after epoch creation, no one would put fund in risk Vault and risk Vault would have 0 TVL. or there are other scenarios where users don't have time to deposit their funds because of the startTime of epoch. in these cases users funds which deposited to other Vault (in pair) would lock in contract forever.

Tools Used

VIM

Recommended Mitigation Steps

check for 0 amount in transfer and prevent calling transfer when amount is zero.

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L144-L192 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L194-L248 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L79-L110

Vulnerability details

Impact

Both functions triggerDepeg() and triggerEndEpoch() are using block.timestamp to detect epoch end time. miners can create value for themself by manipulating block.timestamp, they can cause calls to triggerDepeg() and triggerEndEpoch() to revert in last seconds of epoch by changing block.timestamp if they see they can benefit from it.

Proof of Concept

This is triggerEndEpoch() code:

    /** @notice Trigger epoch end without depeg event
      * @param marketIndex Target market index
      * @param epochEnd End of epoch set for market
      */
    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())
        );
    }

As you can see in the line if(block.timestamp < epochEnd), code checks that if epoch has ended or not, miners can manipulate block.timestamp and cause transactions to this function fail when the transactions was happing in the last seconds of epoch. This is isDisaster() modifier code which is used in triggerDepeg() function:

modifier isDisaster(uint256 marketIndex, uint256 epochEnd) {
        address[] memory vaultsAddress = vaultFactory.getVaults(marketIndex);
        if(
            vaultsAddress.length != VAULTS_LENGTH
            )
            revert MarketDoesNotExist(marketIndex);

        address vaultAddress = vaultsAddress[0];
        Vault vault = Vault(vaultAddress);

        if(vault.idExists(epochEnd) == false)
            revert EpochNotExist();

        if(
            vault.strikePrice() < getLatestPrice(vault.tokenInsured())
            )
            revert PriceNotAtStrikePrice(getLatestPrice(vault.tokenInsured()));

        if(
            vault.idEpochBegin(epochEnd) > block.timestamp)
            revert EpochNotStarted();

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

As you can see in the line if(block.timestamp > epochEnd) code checks that if epoch expired or not, miners can change block.timestamp and cause the transaction to revert in the last seconds of epoch durations. Miners have full control over depegging or ending the epochs in the last seconds and they can choose what ever action that is beneficial for them. for example if miners invested in risk sellers and in the last seconds the price of pegged asset changes too much and others call depegging event, miner can change block.timestamp to cause their transaction to fail and epoch would end without depegging event.

Tools Used

VIM

Recommended Mitigation Steps

add verification phase for epoch depegging event after epochs finished.

Lines of code

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

Vulnerability details

Impact

users deposit their funds in Vault when epoch is not started but as other users deposit funds too or price of pegged token changes users get different risk to reward and they may wants to withdraw their funds before epoch start time to get out of bad position but there is no logic in code to give them ability to withdraw their funds before epoch start time.

Proof of Concept

Withdraw() function in Vault is only allows users to withdraw after epoch ends and their no logics in contract to allow users to withdraw their funds before epoch start time. after users deposit their funds the risk to reward ratio of their investment changes as other users deposit funds in one of the Vaults and user may wants to withdraw their funds if they saw that position is bad for them or maybe the price of that token has been changed dramatically before epoch startTime and users wants to withdraw, but there is no functionality that gives users the ability to withdraw their funds before epoch start time and users lose control of their funds after depositing and before epoch start time. as epoch is not started yet users should be able to withdraw their funds but there is not such functionality in the code.

Tools Used

VIM

Recommended Mitigation Steps

add some logics to give users ability to withdraw funds before epoch start time.

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/VaultFactory.sol#L322-L338 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L283-L289 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L145-L174 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L85-L91

Vulnerability details

Impact

Owner can control timewindow of a Vault and epoch and by that he can control deposits of others and owner can deny other from depositing into a Vault by increasing timewindow as if he saw any profit by that. changing this type of parameters should be done by time-lock mechanism. This is like a backdoor that give owner access to control Vaults.

Proof of Concept

This is changeTimewindow() code in VaultFactory:

    /**
    @notice Admin function, changes vault time window
    @param _marketIndex Target market index
    @param  _timewindow New time window
     */
    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);
    }

As you can see it gives the ability to immediately change timeWindow for any Vault to admin. This is epochHasNotStarted() modifier in Vault contract which is used in deposit() function:

    /** @notice You can only call functions that use this modifier before the current epoch has started
      */
    modifier epochHasNotStarted(uint256 id) {
        if(block.timestamp > idEpochBegin[id] - timewindow)
            revert EpochAlreadyStarted();
        _;
    }

As you can see by changing timewindow it's possible to not allow others to deposit funds to Vaults. So when owner saws that one Vault is very profitable, he can deposit large amount of fund in that risk or hedge Vault and then increase the timewindow for those Vaults so others can't deposit in them and owner can benefit from that deposit and Vaults. and when owner sees there is a profitable Vault that can't deposit in it, he can decrease timewindow to be able to deposit in that Vault. This level of access is like a backdoor that give owner to control deposit allowance to Vaults. This kind of parameters change should only be allowed by time-lock mechanisms.

Tools Used

VIM

Recommended Mitigation Steps

add time-lock mechanisms for changing parameters.

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/rewards/StakingRewards.sol#L212-L223

Vulnerability details

Impact

rewardToken balance in StakingRewards contract is belongs to staking users and owner shouldn't be able to withdraw all balance of rewardToken immediately, because this can cause users to lose their funds if owner perform this action by intention or by mistake. function recoverERC20() gives owner the ability to withdraw rewardToken balance of contract.

Proof of Concept

This is recoverERC20() code:

   // Added to support recovering LP Rewards from other systems such as BAL to be distributed to holders
    function recoverERC20(address tokenAddress, uint256 tokenAmount)
        external
        onlyOwner
    {
        require(
            tokenAddress != address(stakingToken),
            "Cannot withdraw the staking token"
        );
        ERC20(tokenAddress).safeTransfer(owner, tokenAmount);
        emit Recovered(tokenAddress, tokenAmount);
    }

As you can see this code allows owner to transfer rewardToken balance of contract and code only checks that the token is not stakingToken but code should also prevent owner from withdrawing rewardToken too because with this function owner can withdraw rewardToken balance of the contract by intention or by mistake and users who staked would lose their rewards.

Tools Used

VIM

Recommended Mitigation Steps

prevent owner from withdrawing rewardToken.

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/oracles/PegOracle.sol#L39-L83 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/oracles/PegOracle.sol#L85-L106

Vulnerability details

Impact

the code in latestRoundData() don't validate the return values of oracle function to see that the data states are valid, the validation logic implemented in getOracle1_Price() but there is no validation logic for priceFeed1 in the latestRoundData() function. if wrong data has been used by code to calculate prices, then this can cause wrong depeg events and users could lose funds because of this.

Proof of Concept

This is getOracle1_Price() code:

    function getOracle1_Price() public view returns (int256 price) {
        (
            uint80 roundID1,
            int256 price1,
            ,
            uint256 timeStamp1,
            uint80 answeredInRound1
        ) = priceFeed1.latestRoundData();

        require(price1 > 0, "Chainlink price <= 0");
        require(
            answeredInRound1 >= roundID1,
            "RoundID from Oracle is outdated!"
        );
        require(timeStamp1 != 0, "Timestamp == 0 !");

        return price1;
    }

As you can see the code checks and validates the return values of priceFeed1.latestRoundData(). This is latestRoundData() code:

    function latestRoundData()
        public
        view
        returns (
            uint80 roundID,
            int256 nowPrice,
            uint256 startedAt,
            uint256 timeStamp,
            uint80 answeredInRound
        )
    {
        (
            uint80 roundID1,
            int256 price1,
            uint256 startedAt1,
            uint256 timeStamp1,
            uint80 answeredInRound1
        ) = priceFeed1.latestRoundData();

        int256 price2 = getOracle2_Price();

        if (price1 > price2) {
            nowPrice = (price2 * 10000) / price1;
        } else {
            nowPrice = (price1 * 10000) / price2;
        }

        int256 decimals10 = int256(10**(18 - priceFeed1.decimals()));
        nowPrice = nowPrice * decimals10;

        return (
            roundID1,
            nowPrice / 1000000,
            startedAt1,
            timeStamp1,
            answeredInRound1
        );
    }

As you can see there is no check that the return values of priceFeed1.latestRoundData() are valid and code uses the return values to calculate price without validating them and if data was wrong the price is going to be wrong and Controller contract which uses this function to find the pegged asset price and perform depeg event would work incorrectly. users could lose funds because contract would perform depeg event based on wrong data. function latestRoundData() most validate return values of priceFeed1.latestRoundData() like function getOracle1_Price().

Tools Used

VIM

Recommended Mitigation Steps

validate the return values of priceFeed1

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L257-L312

Vulnerability details

Impact

code don't support tokens with more than 18 precision.

Proof of Concept

This is getLatestPrice() code in Controller contract:

    /** @notice Lookup token price
      * @param _token Target token address
      * @return nowPrice Current token price
      */
    function getLatestPrice(address _token)
        public
        view
        returns (int256 nowPrice)
    {
        (
            ,
            /*uint80 roundId*/
            int256 answer,
            uint256 startedAt, /*uint256 updatedAt*/ /*uint80 answeredInRound*/
            ,

        ) = sequencerUptimeFeed.latestRoundData();

        // Answer == 0: Sequencer is up
        // Answer == 1: Sequencer is down
        bool isSequencerUp = answer == 0;
        if (!isSequencerUp) {
            revert SequencerDown();
        }

        // Make sure the grace period has passed after the sequencer is back up.
        uint256 timeSinceUp = block.timestamp - startedAt;
        if (timeSinceUp <= GRACE_PERIOD_TIME) {
            revert GracePeriodNotOver();
        }

        AggregatorV3Interface priceFeed = AggregatorV3Interface(
            vaultFactory.tokenToOracle(_token)
        );
        (
            uint80 roundID,
            int256 price,
            ,
            uint256 timeStamp,
            uint80 answeredInRound
        ) = priceFeed.latestRoundData();

        uint256 decimals = 10**(18-(priceFeed.decimals()));
        price = price * int256(decimals);

        if(price <= 0)
            revert OraclePriceZero();

        if(answeredInRound < roundID)
            revert RoundIDOutdated();

        if(timeStamp == 0)
            revert TimestampZero();

        return price;
    }

As you can see in the line uint256 decimals = 10**(18-(priceFeed.decimals())); code tries to find decimals needed to make price to have exactly 18 precisions. code assumes that he precision is less than 18 and tires to calculate 18-(priceFeed.decimals()) but for token with more than 18 precision this logic would revert and code won't support them.

Tools Used

VIM

Recommended Mitigation Steps

add logic to support both tokens with more than 18 precision or less than 18 precision.