Y2k Finance contest - pauliax's results

Lines of code

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

Vulnerability details

Impact

There are at least a few problems with the PegOracle. I am grouping them into one submission because some of them are not that significant but the last one I believe deserves a higher severity.

1. Function latestRoundData queries getOracle2_Price but getOracle1_Price is not invoked and the price is fetched directly instead:

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

leaving it unvalidated against the stale data and passing this burden up to the caller.

2. Decimals are stored in a signed int type even though it can never be negative:

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

Also, this will revert if the price feed has >18 decimals, it should verify that in the constructor.

3. The main problem with why this has a severity of high is that this contract assumes the default value of decimals is 8, which is not the case: "all pairs have 8 decimals unless it's an ETH pair" (source: https://ethereum.stackexchange.com/a/92513/17387 ). This function will always return 0 when the price feed has 18 decimals. See the proof of concept for more details.

Proof of Concept

I wrote a simple POC that you can try: https://gist.github.com/pauliax/770034a5b744dd2fbba6fa01d9e20eb6

When priceFeed1Decimals is set to 18, then no matter what values are price1 and price2, it will always return 0. Even though this contract fetches the decimals, the code still assumes a default value of 8 which can cause misbehaving.

Recommended Mitigation Steps

Consider fixing the aforementioned issues of the oracle.

Judge has assessed an item in Issue #486 as High risk. The relevant finding follows:

• Contract StakingRewards does not need SafeMath when using the Solidity version starting from 0.8:

  pragma solidity 0.8.15;
  using SafeMath for uint256;
  _balances[msg.sender] = _balances[msg.sender].add(amount);

• When depositing ETH to the vault, users will need an upfront approval of WETH because it wraps the native asset to WETH ERC20 and then calls deposit which respectively calls transferFrom:

  function depositETH(uint256 id, address receiver)
        ...
        IWETH(address(asset)).deposit{value: msg.value}();
        assert(IWETH(address(asset)).transfer(msg.sender, msg.value));
        return deposit(id, msg.value, receiver);
    }
    
  function deposit(
        uint256 id,
        uint256 assets,
        address receiver
    )
        ...
        asset.transferFrom(msg.sender, address(this), shares);

It would be more convenient to adapt these functions so that users won't need extra approval txs.

• In SemiFungibleVault / Vault when isApprovedForAll(owner, receiver) is true then anyone can withdraw from the vault on behalf of the owner:

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

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

I believe that assets should only be withdrawn by the intention of either the owner or the receiver, not by anyone.

• Misleading comment:

    /** @notice You can only call functions that use this modifier after the current epoch has started
      */
    modifier epochHasEnded(uint256 id)

• functions getOracle1_Price and getOracle2_Price are too similar. They should be re-used with different parameters, e.g.:

    function getOracle_Price(AggregatorV3Interface priceFeed) public view returns (int256 price) {
        (
        uint80 roundID,
        int256 price,
        ,
        uint256 timeStamp,
        uint80 answeredInRound
        ) = priceFeed.latestRoundData();

        require(price > 0, "Chainlink price <= 0");
        require(
            answeredInRound >= roundID,
            "RoundID from Oracle is outdated!"
        );
        require(timeStamp != 0, "Timestamp == 0 !");

        return price;
    }

• In PegOracle, decimals are cached in the constructor:

  uint8 public decimals;
  decimals = priceFeed1.decimals();

However, when fetching the price, the decimals are queried again:

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

• No need to recalculate constant values every time the function is invoked, you should extract them to constant variables, e.g. keccak256(abi.encodePacked("rY2K")) never changes here:

  if (
      keccak256(abi.encodePacked(symbol)) ==
      keccak256(abi.encodePacked("rY2K"))
  )

• Admin state variable and modifier onlyAdmin are never used in the Controller. Consider removing them to save some gas.

• Results of repeated calls should be cached and re-used, e.g. here it externally calls vaultFactory.getVaults(marketIndex) twice:

  if(
  vaultFactory.getVaults(marketIndex).length != VAULTS_LENGTH)
      revert MarketDoesNotExist(marketIndex);
  ...
  address[] memory vaultsAddress = vaultFactory.getVaults(marketIndex);