Y2k Finance contest - PwnPatrol'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

While the protocol seems to be trying to account for Chainlink Oracles with different numbers of decimals, the math fails in all cases where decimals does not equal 8.

While this is sufficient for the current tokens used, the protocol has intentionally chosen to allow oracles with any decimal amount (as long as it is equal on both oracles), instead of requiring that decimals = 8, which seems to imply that oracles with different decimals may be used in the future.

Proof of Concept

When new PegOracles are created, the constructor checks that the decimals count for both suboracles is equal, but doesn't specify what that number should be:

require(
    (priceFeed1.decimals() == priceFeed2.decimals()),
    "Decimals must be the same"
);

However, when prices are calculated, the math is only successful if decimals equals 8.

The following code is edited to bring together the math from multiple functions:

nowPrice = (lowerPrice * 10000) / highPrice
int256 decimals10 = int256(10**(18 - priceFeed1.decimals()));
nowPrice = nowPrice * decimals10;

nowPrice = nowPrice / 1e7
uint256 decimals = 10**(18-(priceFeed.decimals()));
price = nowPrice * int256(decimals);

Walking through this step by step:

• nowPrice is set to the ratio of lower / higher * 1e5

• nowPrice is raised to (18-decimals) decimals.

  • If decimals = 8, then that's 10 decimals, bringing total to 1e15
  • If decimals = 18, that's 0, which leaves nowPrice unchanged at 1e5

• nowPrice is divided by 1e7, removing 7 decimals.

  • If decimals = 8, that brings the count down to 1e8
  • If decimals = 18, that zeros out the value

• finally, nowPrice is multiplied by (18-decimals) decimals again

  • If decimals = 8, that brings the count up to 1e18 (intended behavior)
  • If decimals = 18, that leaves the zero value unchanged

This problem holds for any number of decimals except 8, but Chainlink only uses 8 and 18, so it's unlikely any other values would be used.

However, Chainlink uses 18 decimals for all its ETH denominated pairs, which seem likely to be used by the protocol at some point, and will break all the logic.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Either hardcode that decimals must equal 8 in the constructor, or adjust the math to account for all possible decimals lengths as follows: nowPrice = lowerPrice * 1e18 / higherPrice

Lines of code

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

Vulnerability details

Impact

A design flaw in the case of using 2 oracles (aka PegOracle)

Proof of Concept

Chainlink provides price feeds denominated either in ETH or USD. But some assets don't have canonical value accessed on-chain. An example would be BTC and it's many on-chain forms like renBTC, hitBTC, WBTC, aBTC etc... For example in the case of a market on renBTC depegging from BTC value, probably a pair like renBTC/WBTC would be used (leveraging PegOracle). But even if renBTC perfectly maintains it's value to BTC, the depeg event can be triggered when WBTC significantly depreciates or appreciates against BTC value. This depeg event will be theoretically unsound since renBTC behaved as expected. The flaw comes from PegOracle because it treats both assets symmetrically.

This is also true for ETH pairs like stETH/aETH etc.. or stablecoin pairs like FRAX/MIM etc.. Of course, it should never be used like this because Chainlink provides price feeds with respect to true ETH and USD values but we have found that test files include PegOracle for stablecoin pairs...

Tools Used

Manual

Recommended Mitigation Steps

Support markets only for assets that have access to an oracle with price against canonical value x/ETH or x/USD.

Lines of code

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

Vulnerability details

Impact

The approval check in the withdraw() function checks if the receiver of the tokens is approved, not msg.sender, which allows any user to withdraw another user's balance to any address they've approved, including third party applications with no means of retrieval.

This is harmful on its own, but becomes a larger problem if Y2K tokens trade on exchanges like Uniswap, in which case they can be withdrawn into the pool and then "skimmed" by the attacker, allowing theft of funds.

Proof of Concept

When withdraw() is called, it performs the following check that the user calling the function is permitted to do so:

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

This mistakenly checks that the receiver is approved, instead of the msg.sender, which allows a malicious msg.sender to call the function and send the tokens to any approved receiver.

In many cases, the approved receiver will be a protocol with no means of retrieving the funds, leading to permanent loss.

In an extreme case, if Y2K tokens trade on Uniswap, funds can be withdrawn directly to the approved Uniswap pool. The attacker can then call Pair.skim(), which sends the caller the difference between the balances and the reserves. Since this deposit won't impact the reserves, the attacker will receive the newly deposited funds.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Change the approval check to include the following line: isApprovedForAll(owner, msg.sender == false)

Lines of code

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

Vulnerability details

Impact

While the purpose of the protocol is to protect risk assets from depegging to the downside, depeg can be triggered by an increase in value as well.

This poses a problem for Risk Users, as they put up their investment with the expectation that they will only lose funds if the asset depegs to the downside (as confirmed by the sponsor, see attached screenshot).

Proof of Concept

When a Keeper calls triggerDepeg(), part of the validation is to check that the price is below the strike price.

if(vault.strikePrice() < getLatestPrice(vault.tokenInsured()))
    revert PriceNotAtStrikePrice(getLatestPrice(vault.tokenInsured()));

This latest price comes from PegOracle.sol:latestRoundData(), which performs the following calculation:

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

This flips the assets around so that, regardless of which is priced higher, nowPrice captures the discount that the less expensive asset has relative to the more expensive one.

The result is that, if a Risk Asset appreciates sufficiently, triggerDepeg() will succeed and Risk Users will lose their investment.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Specify in the oracle which asset is the risk asset and which is the hedge asset. Once that's clear, nowPrice should be calculated as:

nowPrice = (riskAsset * 10000) / hedgeAsset;

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Vault.sol#L244-L252

Vulnerability details

Impact

Vault interest accrual is broken

Proof of Concept

The protocol is pitched as a market with interest accrual vault (aka ERC4626 standard). This standard is almost entirely implemented in the codebase, however, this implementation has no effect. The totalAssets() is equal to totalSupply() at all times. Thus, there is no interest accrual at all. Actually, the Vault is a very obfuscated equivalent to ERC1155 token.

Tools Used

Manual review

Recommended Mitigation Steps

Either remove all code related to ERC4626 standard if there is no intention to accrue interest or implement interest accrual.

Lines of code

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

Vulnerability details

Impact

Oracles are tracked by individual token, instead of by pair. Because for some tokens (ie BTC) it is unclear which implementation is the canonical one to compare others to, this can result in a situation where different pairs (ie ibBTC-wBTC and ibBC-renBTC) using the same oracle, which will be incorrect for one of them.

Proof of Concept

The protocol assumes that there is a canonical asset to compare pegged assets to, so oracles are tracked only by the pegged asset. However, for some assets (like BTC), there is no clear canonical asset, and the result is that tracking tokenToOracle is not sufficient.

When there is a conflict in tokenToOracle, the protocol responds by skipping the assignment and keeping the old value:

if (tokenToOracle[_token] == address(0)) {
    tokenToOracle[_token] = _oracle;
}

The result of this is that the protocol may define a new pair with a new oracle, and have it silently skip it and use a non-matching oracle. As an example:

• The admins start with an implementation of ibBTC-wBTC, deploying the oracle
• This is set as tokenToOracle[ibBTC]
• Later, the admins create a new pair for ibBTC-renBTC, deploying a new oracle
• The protocol silently skips this assignment and uses the ibBTC-wBTC oracle

This can produce incorrect results, for example if wBTC appreciates relative to the other two, or if both ibBTC and renBTC depeg.

Tools Used

Manual Review, Foundry

Recommended Mitigation Steps

Change tokenToOracle to represent the pair of tokens, either by creating a Pair struct as the key, or by nesting a mapping inside of another mapping.

Lines of code

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

Vulnerability details

Impact

Oracle lacks validation

Proof of Concept

One chainlink oracle usage in PegOracle.sol lacks data validation. https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/oracles/PegOracle.sol#L57-L83

The data is checked later in Vault, however, the function from PegOracle remains public and open for everyone to use and integrate. Others might want to use this data feed and wrongly assume that the data is validated already.

Tools Used

Manual review

Recommended Mitigation Steps

Validate the data feed in PegOracle

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/main/src/Controller.sol#L96-L99

Vulnerability details

Impact

Sensitivity to rapid price change

Proof of Concept

It is actually not rare for some stablecoins like DAI, MIM, FEI, or even USDT to flash-depeg for a very short amount of time. Currently, the protocol doesn't protect RISK users from such brief events.

It would be better if depeg event could be triggered only if the depeg happened for real. This would require either some form of TWAPing.

Tools Used

Manual review

Recommended Mitigation Steps

We recommend protecting RISK users from flash-depegs by utilizing TWAPing or an oracle that supports TWAP price feeds.