Y2k Finance contest - scaraven's results

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/oracles/PegOracle.sol#L67-L82

Vulnerability details

Impact

PegOracle combines two different Chainlink oracles, thereby allowing the protocol to directly compare prices of two different tokens. Chainlink has two main types of oracles, X/USD pairs and X/ETH pairs. The former uses 8 decimals, while the latter uses 18 decimals. PegOracle correctly handles X/USD pairs however incorrectly handles X/ETH pairs and therefore would incorrectly report any price which is derived from ETH pairs.

This will cause ETH pairs to produce prices which are too low and therefore vaults will automatically be registered as depegged even if the underlying assets have not depegged allowing hedge users to unfairly profit.

Proof of Concept

1. Admin deploys a new PegOracle based on ETH-pairs where decimals is initialised to be 18

        decimals = priceFeed1.decimals();

2. When latestRoundData() is called, nowPrice is calculated and has 4 decimals due to price2 and price1 cancelling each other's decimals

            //@audit 18 + 4 - 18
            nowPrice = (price2 * 10000) / price1;

3. As priceFeed1.decimals() has 18 decimals, decimals10 becomes 1

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

4. This means that nowPrice stays at 4 decimals and when it is returned it now has -2 decimals even though PegOracle.decimals() = 18

            //4 - 6 = -2 decimals
            nowPrice / 1000000

5. Therefore, for all relative prices under 100x, price rounds down to 0 which is incorrect

The current formula for the number of decimals of the returned price is 16 - x where x = priceFeed1.decimals(). Therefore, for 8 decimals (USD pairs) PegOracle is correct.

Tools Used

VS Code

Recommended Mitigation Steps

Rewrite latestRoundData() to:

    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 * decimals) / price1;
        } else {
            nowPrice = (price1 * decimals) / price2;
        }



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

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L217 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/rewards/StakingRewards.sol#L99-L105 https://github.com/OpenZeppelin/openzeppelin-contracts/blob/7a14f6c5953a1f2228280e6eb1dfee8e5c28d79a/contracts/token/ERC1155/ERC1155.sol#L125

Vulnerability details

Impact

In Vault.sol, after an epoch has ended (or a depeg event has occurred), users can withdraw() on behalf of other users as long as the receiver of asset has been approved by the owner. Such receiver's can also include StakingRewards.sol if a user has staked their assets because users must approve the StakingRewards contract so that they can stake(). Therefore, a malicious user can burn ERC1155 shares of users who have staked and send the resulting funds to the StakingRewards contract.

In order for this to work, users must have staked and withdrawn from StakingRewards without withdrawing from Vault.sol (though it is not impossible that an attacker frontruns the second action) Funds are not permanently lost as the asset funds can be recovered through recoverERC20() but this is dependent on the owner to properly distribute funds back to affected users. Alternatively, the owners can use this to steal funds for themselves as an indirect rug pull vector.

Proof of Concept

1. Alice takes part in a market and invests 100 WETH into a vault and receives 100 ERC1155 tokens
2. Alice approves StakingRewards and stakes her tokens into StakingRewards through stake()
3. After a certain amount of time, Alice exits her position and withdraws her ERC1155 tokens from StakingRewards
4. It is unlikely Alice has revoked her approval and so Bob calls withdraw() on behalf of Alice where recipient is StakingRewards
5. Alice's funds from the vault are sent to StakingRewards where they are temporarily locked

Tools Used

VS Code

Recommended Mitigation Steps

Consider disallowing users who have no approval to withdraw on behalf of other users. Instead, only allow the owners or approved operators e.g.

        if(
            msg.sender != owner &&
            isApprovedForAll(owner, msg.sender) == false)
            revert OwnerDidNotAuthorize(msg.sender, owner);

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/oracles/PegOracle.sol#L58-L63 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L308-L309 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/oracles/PegOracle.sol#L103

Vulnerability details

Impact

In PegOracle.latestRoundData(), when priceFeed1.latestRoundData() is called there are no checks whether the data is stale or not. This could lead to the oracle reporting incorrect prices which can negatively affect hedge and risk users, e.g. the oracle reports a price of 1.0 even if a depeg has occurred.

Additionally, even though all other sections of code which use oracles do check that answeredInRound >= round, it may be the case that the chain link nodes have stopped working and therefore not updated to a new round in a very long time causing the data to be stale.

Tools Used

VS Code

Recommended Mitigation Steps

Add checks for staleness of data including checking the timestamp of the data e.g.

            uint80 roundID1,
            int256 price1,
            ,
            uint256 timeStamp1,
            uint80 answeredInRound1
        ) = priceFeed1.latestRoundData();


        require(price1 > 0, "Chainlink price <= 0");
        require(
            answeredInRound1 >= roundID1,
            "RoundID from Oracle is outdated!"
        );
        //@audit make sure that the timestamp is not from too long ago
        require(timeStamp1 > block.timestamp - OracleWindow, "Timestamp is too low");

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/VaultFactory.sol#L295-L374

Vulnerability details

Impact

Throughout most contracts, a single user is given control over changing important parameters e.g. VaultFactory.setController(). If this user is at any point malicious, then it is very easy for the user to manipulate parameters in a way to steal funds from normal users e.g. setting a malicious oracle which incorrectly reports a depeg

Recommended Mitigation Steps

It is highly recommended to either implement delays when changing parameters so that users have time to react to changes which may seem malicious and/or making sure that a multi-sig wallet or DAO control the contracts