Y2k Finance contest - ladboy233's results

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

The oracle is used to determine if the token is a peg or debegged in vaults so the oracle is a mission-critical component in the application,

let's look into the current implementation of the oracle

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

first of all,

there is risk that the oracle is rounded to 0 in the code below,

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

multiple by 10000 does not ensure that the denominator is smaller than the numerator.

let's look into the decimal conversion

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

what if the price decimal is 18, the decimal10 would be 1 and the price oracle is not accurate.

In the getLastestPrice funtion from the controller,

we have the inaccurate decimal conversion again

        (
            uint80 roundID,
            int256 price,
            ,
            uint256 timeStamp,
            uint80 answeredInRound
        ) = priceFeed.latestRoundData();

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

the line

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

cannot be reliable due the decimal conversion.

Why 10**(18-(priceFeed.decimals())) is a issue?

because the oracle price is used to compare against the strike price in controller

         if(
            vault.strikePrice() < getLatestPrice(vault.tokenInsured())
        )

what is strikePrice, strike price is set in the Vault constructo from the function createNewMarket in VaultFactory

    /**
    @notice Function to create two new vaults, hedge and risk, with the respective params, and storing the oracle for the token provided
    @param _withdrawalFee uint256 of the fee value, multiply your % value by 10, Example: if you want fee of 0.5% , insert 5
    @param _token Address of the oracle to lookup the price in chainlink oracles
    @param _strikePrice uint256 representing the price to trigger the depeg event, needs to be 18 decimals
    @param  epochBegin uint256 in UNIX timestamp, representing the begin date of the epoch. Example: Epoch begins in 31/May/2022 at 00h 00min 00sec: 1654038000
    @param  epochEnd uint256 in UNIX timestamp, representing the end date of the epoch. Example: Epoch ends in 30th June 2022 at 00h 00min 00sec: 1656630000
    @param  _oracle Address representing the smart contract to lookup the price of the given _token param
    @return insr    Address of the deployed hedge vault
    @return rsk     Address of the deployed risk vault
     */
    function createNewMarket(
        uint256 _withdrawalFee,
        address _token,
        int256 _strikePrice,
        uint256 epochBegin,
        uint256 epochEnd,
        address _oracle,
        string memory _name
    )

according to the comment

     @param _strikePrice uint256 representing the price to trigger the depeg event, needs to be 18 decimals

but

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

do not give us the price in 18 decimals. so the price can be very undervalued.

Tools Used

Foundary

Manual

Recommended Mitigation Steps

I think the protocol can refer to this oracle implementation

https://github.com/opynfinance/squeeth-monorepo/blob/525e3dcfa79330716100f8922e3316ac845a9fa1/packages/hardhat/contracts/core/Oracle.sol#L105

    /**
     * @notice get twap converted with base & quote token decimals
     * @dev if period is longer than the current timestamp - first timestamp stored in the pool, this will revert with "OLD"
     * @param _pool uniswap pool address
     * @param _base base currency. to get eth/usd price, eth is base token
     * @param _quote quote currency. to get eth/usd price, usd is the quote currency
     * @param _period number of seconds in the past to start calculating time-weighted average
     * @return twap price which is scaled
     */
    function _fetchTwap(
        address _pool,
        address _base,
        address _quote,
        uint32 _period
    ) internal view returns (uint256) {
        uint256 quoteAmountOut = _fetchRawTwap(_pool, _base, _quote, _period);

        uint8 baseDecimals = IERC20Detailed(_base).decimals();
        uint8 quoteDecimals = IERC20Detailed(_quote).decimals();
        if (baseDecimals == quoteDecimals) return quoteAmountOut;

        // if quote token has less decimals, the returned quoteAmountOut will be lower, need to scale up by decimal difference
        if (baseDecimals > quoteDecimals) return quoteAmountOut.mul(10**(baseDecimals - quoteDecimals));

        // if quote token has more decimals, the returned quoteAmountOut will be higher, need to scale down by decimal difference
        return quoteAmountOut.div(10**(quoteDecimals - baseDecimals));
    }

Lines of code

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

Vulnerability details

Impact

in the AssertTest.t.sol test, for the test function testOwnerAuthorize,

there is a comment above

    //@dev: please assess this case and check if contract logic fault or test logic fault

if we run the test

forge test --match OwnerAuthorize  --fork-url https://arb1.arbitrum.io/rpc -vv

the error message would be

Encountered 1 failing test in test/AssertTest.t.sol:AssertTest
[FAIL. Reason: EvmError: Revert] testOwnerAuthorize() (gas: 6285852)

in this case, the test fails because we call the accounting function beforeWithdraw inside the function withdraw

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

in the function beforeWithdraw, division by zero is possible, which revert the transaction and in this case, block withdraw

    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 {

            return idFinalTVL[id];

            entitledAmount = amount.divWadDown(idFinalTVL[id]).mulDivDown(
                idClaimTVL[id],
                1 ether
            );
        }

        return entitledAmount;
    }

the line

amount.divWadDown(idFinalTVL[id])

revert because isFinalTVL[id] could be 0.

Proof of concepts

If we add

    return idFinalTVL[id];

above

     entitledAmount = amount.divWadDown(idFinalTVL[id]).mulDivDown(
           idClaimTVL[id],
           1 ether
       );

in the beforeWithdraw function

forge test --match OwnerAuthorize  --fork-url https://arb1.arbitrum.io/rpc -vv

pass, showing the division by zero issue block the user from withdrawing the share.

Tools Used

Manual Review Foundry

Recommended Mitigation Steps

We recommend the verify the denominator is not zero to make sure the transaction does not revert.

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding.

let's look into the implementation of the latestRoundata() in PegOracle.sol

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

        int256 price2 = getOracle2_Price();

in getOracle2_price(), we check if the getOracle2_price() is outdated.

    function getOracle2_Price() public view returns (int256 price) {
        (
            uint80 roundID2,
            int256 price2,
            ,
            uint256 timeStamp2,
            uint80 answeredInRound2
        ) = priceFeed2.latestRoundData();

        require(price2 > 0, "Chainlink price <= 0");
        require(
            answeredInRound2 >= roundID2,
            "RoundID from Oracle is outdated!"
        );
        require(timeStamp2 != 0, "Timestamp == 0 !");

        return price2;
    }

same check is in the function getOracle1_price()

but instead of using getOracle1_price(),

the function does not check if timestamp and round id is valid,

the function use price1 to compute the price now directly.

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

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

using outdated price or invalid price can result in the invalid depeg or peg of the risk vault and hedge vault at the loss of user.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

1. the price2 oracle is update to date.
2. the price1 oracle is not updated.
3. the oracle use outdated price1 to compute price now result in the false depeg of the vault

Tools Used

Foundry Manual Review

Recommended Mitigation Steps

We recommand the project to use getOracle1_Price() because it is already implemented below.

Unused onlyAdmin modifier in Controller.sol

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Controller.sol#L73

    modifier onlyAdmin() {
        if(msg.sender != admin)
            revert AddressNotAdmin();
        _;
    }

is not used. Please remove

Use require instead of assert in Vault.sol when handling WETH transfer

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Vault.sol#L190

   IWETH(address(asset)).deposit{value: msg.value}();
   assert(IWETH(address(asset)).transfer(msg.sender, msg.value));

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol

We recommand the project use SafeERC20 and use safeTransfer,

or replace assert with require.

Unhandled external return value when calling transfer and transferFrom in Vault.sol

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Vault.sol#L167

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Vault.sol#L228

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Vault.sol#L231

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Vault.sol#L365

when calling transfer or transferFrom in Vault.sol

      asset.transfer(_counterparty, idFinalTVL[id]);

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

the return value is not handled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/utils/SafeERC20.sol

We recommand the project use SafeERC20 and use safeTransfer,

Increase test coverage and make sure the test pass

test fails when running test.

by running

 forge test

the result is

Encountered a total of 20 failing tests, 32 tests succeeded

by running

 forge test --match-contract AssertTest --fork-url https://arb1.arbitrum.io/rpc -vv

the result is

Encountered a total of 1 failing tests, 16 tests succeeded

the test fails.

Please make sure proper test is implemented to increase the test coverage.

Use transfer instead of transferFrom in StakingRewards.sol

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

instead of

  stakingToken.safeTransferFrom(
            address(this),
            msg.sender,
            id,
            amount,
            ""
    );

we can write

  stakingToken.safeTransfer(
            msg.sender,
            id,
            amount,
            ""
    );

Outdated Openzeppelin version

the openzepplein version used is "version": "4.6.0",

however, the most updated openzeppelin version is 4.7.3

there are known vulnerabliilty in outdated version including version 4.6.0.

https://security.snyk.io/package/npm/@openzeppelin%2Fcontracts

please use the up to date code version

getNextEpoch in Vault.sol is not used and has potential index out of range error

https://github.com/code-423n4/2022-09-y2k-finance/blob/ac3e86f07bc2f1f51148d2265cc897e8b494adf7/src/Vault.sol#L438

    function getNextEpoch(uint256 _epoch)
        public
        view
        returns (uint256 nextEpochEnd)
    {
        for (uint256 i = 0; i < epochsLength(); i++) {
            if (epochs[i] == _epoch) {
                if (i == epochsLength() - 1) {
                    return 0;
                }
                return epochs[i + 1];
            }
        }
    }

the function above is not used anywhere.

the line

 return epochs[i + 1]; 

can throw index out of range error.

Redundant safeMath usage in StakingReward.sol

the solidtity version above 0.8.0 handles overflow and underflow in arithmitic operation so no need to use safe math.

Confusing name in Vault.sol in deposit function

the variable assets, according to the comment

      @param  assets  uint256 representing how many assets the user wants to deposit, a fee will be taken from this value;

in the function,

    function deposit(
        uint256 id,
        uint256 assets,
        address receiver
    )

however the name is confusing because assets make people think it is a list of token asset. We recommand the project change the assets name to assetsAmount.

missing zero address check in Vault.sol in deposit and withdraw function for receiver

we recommand the project check if the receiver of the shares is address(0)

missing Event emission when changing crucial smart contract state in Vault.sol

the function below does not emit event when changing critical state.

    function changeTreasury(address _treasury) public onlyFactory {

    function changeTimewindow(uint256 _timewindow) public onlyFactory {

       function changeController(address _controller) public onlyFactory {

      function createAssets(uint256 epochBegin, uint256 epochEnd, uint256 _withdrawalFee)

invalid link in comment in StakingRewards.sol

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

in stakingRewards.sol

the link in comment is

// https://docs.synthetix.io/contracts/source/contracts/stakingrewards,

but the correct link is

https://docs.synthetix.io/contracts/source/contracts/StakingRewards/