Y2k Finance contest - 0xPanas's results

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L28-L36 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L67-L78 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Controller.sol#L291-L300

Vulnerability details

PegOracle will return wrong prices when decimals() are less than 18

Impact

Code will return more price as less value is returned in decimals()

Proof of Concept

First of all, decimals() doesn't need to return exactly 18 decimals, can return more or less than 18 too. However, only is checked that both decimal amounts are the same.

        require(
            (priceFeed1.decimals() == priceFeed2.decimals()),
            "Decimals must be the same"
        );//@audit both can be more than 18 decimals

        oracle1 = _oracle1;
        oracle2 = _oracle2;

        decimals = priceFeed1.decimals();

https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L28-L36

nowPrice is increased by 10_000 for precision

 
 if (price1 > price2) {//@audited even of the condition 0 can happen on else or double 0
            nowPrice = (price2 * 10000) / price1;//@audit here
        } else {
            nowPrice = (price1 * 10000) / price2;//@audit here
        }

        int256 decimals10 = int256(10**(18 - priceFeed1.decimals()));
        nowPrice = nowPrice * decimals10;//@audit if decimals10 is more than 100, code will return more value when divided by 1_000_000

        return (
            roundID1,
            nowPrice / 1_000_000,//@audit this value is hardcoded and doesn't correspond to the value to normalize again the previous multiplication.
            startedAt1,
            timeStamp1,
            answeredInRound1
        );

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

As decimals() is less than 18, it creates a exponentiation than after being used for multiply nowPrice is tried to be converted back by dividing by a hardcoded value of 1_000_000.

If decimals() for example returns a value of 6, let's assume:

• price2: 3
• price1: 3
• priceFeed1.decimals(): 6
• nowPrice would be: 30_000 / 3 = 10_000
• int256 decimals10 = int256(10**(18 - priceFeed1.decimals()));
• this would give as result: decimals10 = 10**(12)
• then nowPrice = 10_000 * 10**12 = 10_000_000_000_000_000
• finally return value for price would be: 10_000_000_000_000_000 / 1_000_000 = 10_000_000_000

and finally this price is also used in Controller.sol

  (
      uint80 roundID,
      int256 price, //@audit returned here previous value
      ,
      uint256 timeStamp,
      uint80 answeredInRound
  ) = priceFeed.latestRoundData();

  uint256 decimals = 10**(18-(priceFeed.decimals()));//@audit decimals neither checked
  price = price * int256(decimals);

https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Controller.sol#L291-L300

And can be seen the same issue with priceFeed.decimals()

Mitigation

• Check priceFeed and other tokens used to have expected decimals.
• Change hardcoded normalization of value to be able to deal with less than 18 decimals

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L70-L74 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L175-L178

Vulnerability details

Divide before multiply

Impact

Solidity integer division might truncate. As a result, performing multiplication before division can sometimes avoid loss of precision.

Proof Of Concept

In general, this is a problem due to precision. In this case, it also affects money/tokens, what makes me suggest medium severity.

There are 2 instances of this:

• PegOracle.latestRoundData() This would affect the price recovered by latestRoundData().

    if (price1 > price2) {
        nowPrice = (price2 * 10000) / price1;//@audit divide
    } else {
        nowPrice = (price1 * 10000) / price2; //@audit divide
    }
  //then multiply
	nowPrice = nowPrice * decimals10

https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L70-L74

• StakingRewards.earned() This is a little more tricky to see and affects returned earned amount

    function earned(address account) public view returns (uint256) {
        return
            _balances[account]
                .mul(rewardPerToken().sub(userRewardPerTokenPaid[account]))
                .div(1e18)
                .add(rewards[account]);//@audit add should be used before div for max precision
    }

Let $b_\text{account}$ be _balances[account] $R_t$ be rewardPerToken() $uR_\text{account}$ be userRewardPerTokenPaid[account] $R_\text{account}$ be rewards[account] then, consider implementing the computation of earned as follows:

https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L175-L178

Tools

Slither + manual analysis

Recommended Mitigation Steps

Reorder the operations. For more info

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L157 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L163-L374

Vulnerability details

Immutable address owner is risky in VaultFactory.sol

Impact

The following contracts and functions, allow admins to interact with core functions such as:

VaultFactory.sol functions for:

• createNewMarket
• deployMoreAssets
• setController
• changeTreasury
• changeTimewindow
• changeController
• changeOracle

Given that admin is immutable it's very risky because it is irrecoverable from any mistakes

Scenario: If an incorrect address, e.g. for which the private key is not known, is used accidentally then it prevents the use of all the onlyAdmin() functions forever, which includes the changing of various critical addresses and parameters. This use of incorrect address may not even be immediately apparent given that these functions are probably not used immediately.

When noticed, due to a failing onlyAdmin() function call, it will force the redeployment of these contracts and require appropriate changes and notifications for switching from the old to new address. This will diminish trust in the protocol and incur a significant reputational damage.

Github Permalinks

https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L157

• Admin functions affected https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L163-L374

Recommended steps

Recommend remove immutable from admin address. Also taking care while deploying the contract / emitting an event when assigning _admin so in case it is wrongly deployed, it can be redeployed earlier. Finally adding a 2 steps transfer from admin for cases when admin needs to be migrate to another address.