Y2k Finance contest - hyh's results

Lines of code

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

Vulnerability details

The dependency on Oracle decimals isn't stated anywhere, however, current solution will work with 8 decimals (i.e. USD based in the Chainlink case) feeds only:

• 8 decimals feed: nowPrice is initially out of 10000, i.e. have decimals of 4, then corrected to be 4 + (18-8) = 14 decimals, then divided by 1e6 to become 8 decimals

• 18 decimals feed, say stETH/ETH and flat 1e18 for ETH: nowPrice have decimals of 4, decimals10 is 1e0, so nowPrice / 1000000 is 0 all the time

This way, for example, for 18 decimals feeds, PegOracle' latestRoundData() will permanently report zero price.

Proof of Concept

Prices are treated as having 8 decimals, i.e. the * 10000, * 10**(18 - priceFeed1.decimals()) and / 1000000 formulas only work when priceFeed1 and priceFeed2 have 8 decimals, i.e. USD based in Chainlink case:

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

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

This way the logic is de facto hard coded for the 8 decimals pegged and underlying assets feeds, which isn't always the case and there is no such limitation stated:

https://github.com/code-423n4/2022-09-y2k-finance#oraclespegoraclesol---high

In order to PegOracle to work with all price feeds, its logic needs to be unified a bit.

Recommended Mitigation Steps

nowPrice can be set to the desired scale on construction, there is no need to do anything else. The scale can be set to be constant or to be equal to initial feed's scale.

For the constant scale case, FRACTION_DECIMALS, as an example:

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

    uint8 public decimals;
+    
+   uint8 immutable public FRACTION_DECIMALS = 8;

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

+		uint8 price1Decimals = priceFeed1.decimals();
+		uint8 price2Decimals = priceFeed2.decimals();
+		int256 outScale = FRACTION_DECIMALS + price1Decimals - price2Decimals;
        if (price1 > price2) {
-           nowPrice = (price2 * 10000) / price1;
+           nowPrice = outScale > 0 ? (price2 * 10**outScale) / price1 : price2 / (price1 * 10**(-outScale));
        } else {
-           nowPrice = (price1 * 10000) / price2;
+           nowPrice = outScale > 0 ? (price1 * 10**outScale) / price2 : price1 / (price2 * 10**(-outScale));
        }

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

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

This will also improve the nowPrice precision from 4 to FRACTION_DECIMALS.

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/SemiFungibleVault.sol#L110-L119 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L203-L218

Vulnerability details

Anyone can withdraw to receiver once the receiver is isApprovedForAll(owner, receiver). The funds will be sent to receiver, but it will happen whenever an arbitrary msg.sender wants. The only precondition is the presence of any approvals.

This can be easily used to sabotage the system as a whole. Say there are two depositors in the hedge Vault, Bob and David, both trust each other and approved each other. Mike the attacker observing the coming end of epoch where no depeg happened, calls the withdraw() for both Bob and David in the last block of the epoch. Mike gained nothing, while both Bob and David lost the payoff that was guaranteed for them at this point.

Setting the severity to be high as this can be routinely used to sabotage the y2k users, both risk and hedge, depriving them from the payouts whenever they happen to be on the winning side. Usual attackers here can be the users from the another side, risk users attacking hedge vault, and vice versa.

Proof of Concept

isApprovedForAll() in withdrawal functions checks the receiver to be approved, not the caller.

SemiFungibleVault's withdraw:

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/SemiFungibleVault.sol#L110-L119

    function withdraw(
        uint256 id,
        uint256 assets,
        address receiver,
        address owner
    ) external virtual returns (uint256 shares) {
        require(
            msg.sender == owner || isApprovedForAll(owner, receiver),
            "Only owner can withdraw, or owner has approved receiver for all"
        );

Vault's withdraw:

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

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

This way anyone at any time can run withdraw from the Vaults whenever owner has some address approved.

Recommended Mitigation Steps

Consider changing the approval requirement to be for the caller, not receiver:

SemiFungibleVault's withdraw:

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/SemiFungibleVault.sol#L110-L119

    function withdraw(
        uint256 id,
        uint256 assets,
        address receiver,
        address owner
    ) external virtual returns (uint256 shares) {
        require(
-           msg.sender == owner || isApprovedForAll(owner, receiver),
+           msg.sender == owner || isApprovedForAll(owner, msg.sender),
            "Only owner can withdraw, or owner has approved receiver for all"
        );

Vault's withdraw:

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

    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)
+           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#L63-L71

Vulnerability details

Depeg event is defined as linked asset price being below the strike price in the terms of the underlying asset. However, the PegOracle aimed to report the fraction of the pegged asset to the underlying always reports the number below 1, no matter how prices are staked against each other, the lesser one is divided by the bigger one.

There is no indicator whether price2 / price1 or price1 / price2 is used, so < 1 fraction is always reported.

Say if stETH was 0.99, then flipped ETH for any reason, becoming 1.1 (say an additional reward program was announced by Lido and the market priced the present value of these rewards to be 10%). PegOracle will report the stETH/ETH fraction as 1 / 1.1 = 0.909.

This, for example, can allow triggerDepeg() to be run if strikePrice were set as 0.91.

Setting the severity to be high as that's the violation of core logic with substantial fund loss for all hedge users.

Proof of Concept

PegOracle's latestRoundData() do not separate linked and underlying price feeds, reporting the fraction of the currently smaller one to the bigger:

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

        ) = priceFeed1.latestRoundData();

        int256 price2 = getOracle2_Price();

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

Recommended Mitigation Steps

Consider fixing the order of the price feeds and always report linked asset price divided by the underlying asset price.

For example:

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

    /** @notice Contract constructor
-     * @param _oracle1 First oracle address
-     * @param _oracle2 Second oracle address
+     * @param _oracle1 Linked oracle address
+     * @param _oracle2 Underlying oracle address
      */
    constructor(address _oracle1, address _oracle2) {
-       require(_oracle1 != address(0), "oracle1 cannot be the zero address");
-       require(_oracle2 != address(0), "oracle2 cannot be the zero address");
+       require(_oracle1 != address(0), "Linked Oracle is zero address");
+       require(_oracle2 != address(0), "Underlying Oracle is zero address");

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

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

Lines of code

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

Vulnerability details

There are no checks for price1 obtained from Chainlink's priceFeed1 in PegOracle's latestRoundData(), so the resulting fraction latestRoundData() returns will be stale or just representing an error once price1 malfunctions, which isn't controlled now.

Using stale prices in the business logic can lead to monetary losses for the users as decision making be triggered by erroneous values.

Proof of Concept

Round and timestamp data is retrieved, but ignored, i.e. price1 value used all the time as returned, without checks:

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

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

        int256 price2 = getOracle2_Price();

        if (price1 > price2) {

priceFeed1 result can be invalid in several ways, for example price1 can be zero or negative or timeStamp1 can be zero. Here the price is used without any checks, that will lead to malfunctions in the business logic that uses PegOracle's feed.

Recommended Mitigation Steps

The checks are already implemented in the getOracle1_Price() function, so consider using it instead of the raw request:

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

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

+       int256 price1 = getOracle1_Price();


        int256 price2 = getOracle2_Price();

        if (price1 > price2) {

And returning all the fields there:

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

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

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

-       return price1;
    }