Y2k Finance contest - Jeiwan's results

Lines of code

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

Vulnerability details

Impact

PegOracle reports wrong prices when the underlying oracle use decimals other than 8. A price reported by PegOracle can trigger a depeg event immediately after an epoch has started.

Proof of Concept

function testPegOracleDecimals() public {
    DepegOracle ethOracle = new DepegOracle(address(oracleETH), address(admin));
    DepegOracle stethOracle = new DepegOracle(address(oracleSTETH), address(admin));
    ethOracle.setDecimals(14);
    stethOracle.setDecimals(14);

    // A peg oracle for ETH/stETH (ETH/USD and stETH/USD)
    PegOracle pegOracle = new PegOracle(address(ethOracle), address(stethOracle));

    // ETH/stETH is 1
    vm.startPrank(admin);
    ethOracle.setPriceSimulation(5000e14);
    stethOracle.setPriceSimulation(5000e14);
    vm.stopPrank();
    (,int256 nowPrice, , ,) = pegOracle.latestRoundData();

    // Expected: 1e14, actual: 100
    assertEq(nowPrice, 1e14);
}

In the above case, the underlying oracles use 14 decimals; the prices they report also use 14 decimals. It's expected that the PegOracle reports 1e14, but it reports 100–this is way below any reasonable strike price. The Controller won't be able to scale the price: it will multiply 100 by 10**(18-14), which equals 100e4, not 1e14.

While most Chainlink oracles use 8 decimals, some of them use 18 (all ETH pairs, e.g. LINK/ETH).

Recommended Mitigation Steps

In the latestRoundData function of PegOracle, scale the final price to match the decimals of the underlying oracles.

Lines of code

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

Vulnerability details

Impact

Vault assets get locked forever when one of the vaults has 0 TVL and a depeg event happens. Either risk buyers (when the hedge vault has 0 TVL) or insurance buyers (when the risk vault has 0 TVL) get 0 reward and their funds get locked forever in the opposite vault.

Proof of Concept

function testUnrecoverableAssets() public {
    vm.deal(alice, 10 ether);

    // Setting up markets...
    DepegOracle depegOracle = new DepegOracle(address(oracleDAI), address(admin));
    vm.startPrank(admin);
    vaultFactory.createNewMarket(FEE, tokenDAI, DEPEG_AAA, beginEpoch, endEpoch, address(depegOracle), "y2kDAI_99*");
    vm.stopPrank();

    Vault hedge = Vault(vaultFactory.getVaults(1)[0]);
    Vault risk = Vault(vaultFactory.getVaults(1)[1]);

    // Alice deposits to the risk vault.
    vm.startPrank(alice);
    ERC20(WETH).approve(address(risk), 1 ether);
    risk.depositETH{value: 1 ether}(endEpoch, alice);
    vm.stopPrank();

    // No deposits were made to the hedge vault.

    // Oracle reports a depegged price.
    vm.warp(beginEpoch + 1);
    vm.prank(admin);
    depegOracle.setPriceSimulation(DEPEG_AAA - 1);

    // Controller triggers a depeg event. Vaults' assets get swapped.
    controller.triggerDepeg(1, endEpoch);

    vm.prank(alice);
    uint256 withdrawnAmount = risk.withdraw(endEpoch, 1 ether, alice, alice);

    // Alice gets nothing even though risk buyers are expected to get some
    // reward.
    assertEq(withdrawnAmount, 0);

    // Unrecoverable funds since there were no insurance buyers.
    assertEq(ERC20(WETH).balanceOf(address(hedge)), 1 ether);
    assertEq(hedge.totalAssets(endEpoch), 0);
}

According to the design, both risk and insurance buyers bet on the opposite outcomes of a depeg event. When a depeg happens, vaults' assets get swapped and one of the parties gets a profit; the other gets a loss. However, when one of the vaults has 0 TVL, the other party loses all its assets (they're transferred to the other vault), gets nothing in exchange (the other vault has 0 assets), and its funds get locked forever in the other vault (no one can withdraw them because idFinalTVL is 0 here and here in the other vault).

With many stablecoins and vaults, and with tight strike prices, the likelihood of such events doesn't seem too low. Even though it's not high, what raises the severity of such situations is that funds become unrecoverable and vault depositors are left with nothing.

Recommended Mitigation Steps

Consider cancelling epochs that have 0 assets in either vault and allowing depositors to withdraw full amounts early, without waiting for endEpoch.

Lines of code

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

Vulnerability details

The peg oracle always reports prices that are less or equal to 1. This means that the vaults that will use the oracle will have a higher chance of a depeng than the standard Chainlink oracles. Eventually, this will put risk buyers at a disadvantage and will make them lose money more often.

Proof of Concept

function testPegOracleUnfairness() public {
    DepegOracle ethOracle = new DepegOracle(address(oracleETH), address(admin));
    DepegOracle stethOracle = new DepegOracle(address(oracleSTETH), address(admin));
    ethOracle.setDecimals(8);
    stethOracle.setDecimals(8);

    // A peg oracle for ETH/stETH (ETH/USD and stETH/USD)
    PegOracle pegOracle = new PegOracle(address(ethOracle), address(stethOracle));

    // ETH/stETH is 1
    vm.startPrank(admin);
    ethOracle.setPriceSimulation(5000e8);
    stethOracle.setPriceSimulation(5000e8);
    vm.stopPrank();
    (,int256 nowPrice, , ,) = pegOracle.latestRoundData();
    assertEq(nowPrice, 100000000);

    // ETH/stETH is below 1
    vm.startPrank(admin);
    ethOracle.setPriceSimulation(5000e8);
    stethOracle.setPriceSimulation(4000e8);
    vm.stopPrank();
    (,nowPrice, , ,) = pegOracle.latestRoundData();
    assertEq(nowPrice, 80000000);

    // ETH/stETH is below 1 again
    vm.startPrank(admin);
    ethOracle.setPriceSimulation(4000e8);
    stethOracle.setPriceSimulation(5000e8);
    vm.stopPrank();
    (,nowPrice, , ,) = pegOracle.latestRoundData();
    assertEq(nowPrice, 80000000);
}

PegOracle works with two Chainlink oracles and calculates a price of one asset (e.g. ETH) in terms of the other (e.g. stETH) through a common asset they're both traded against (e.g. USD). However, PegOracle doesn't keep record of which of the two assets is the base asset (ETH in the ETH/stETH pair) and which is the pegged asset (stETH in the ETH/stETH pair): it always divides the more expensive asset by the cheaper one. As a result, the prices it reports can never be greater than 1:

1. if ETH is $5000 and stETH is $4000, PegOracle will report 0.8;
2. If ETH is $4000 and stETH is $5000, PegOracle will also report 0.8.

This means that PegOracle will report depegged prices more often than the standard Chainlink oracles. Since vaults have only one strike price, the vaults that will use PegOracle will have a higher rate of depegs than Chainlink oracles.

Recommended Mitigation Steps

In PegOracle, consider one oracle as the base asset oracle and the other oracle as the pegged asset oracle; compute and return the price of the pegged asset in terms of the base asset.

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

Lines of code

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

Vulnerability details

Impact

A wrong price might be reported by PegOracle when a Chainlink oracle reports an invalid price (e.g. 0) or when the price is outdated.

Proof of Concept

The values returned by the priceFeed1.latestRoundDate() are not validated. Interestingly, there's getOracle1_Price function that gets a price from the same oracle and validates the returned values. It seems like the function is mistakenly not used in the latestRoundData of PriceOracle.

Recommended Mitigation Steps

Use getOracle1_Price to get a price from oracle1 instead of calling it directly.

[L-01] Inaccurate comment

Targets

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

Description

The comment says: "multiply by 1000 then divide by 5", however, the code does the opposite: it multiplies by a fee and divides by 1000.

[L-02] Missing marketExists modifier

Targets

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

Description

To ensure that claim TVL cannot be added to an non-existent market, consider adding the missing modifier.

[L-03] Missing epochHasEnded modifier

Targets

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

Description

To ensure funds can never be transferred out of the vault until an epoch is over, consider adding the missing modifier.

[N-01] Vault doesn't implement EIP-4626

Targets

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

Description

Vault doesn't implement all the functions from EIP-4626. Specifically, mint and redeem are missed.

What's more essential is that Vault, in its core, is not a vault as it's defined in EIP-4626. The main lacking feature is the conversion between shares and assets: Vault treats them equally. This makes useless the usage of previewWithdraw and previewDeposit functions because they always return the input amount:

1. previewDeposit calls convertToShares, which converts asset to shares. But since asset are shares, it always returns assets. supply always equals totalAssets(id), thus the returned values is always assets;
2. in previewWithdraw, the logic is the same.

Tokenized Vaults, as per EIP-4626, need to track assets and shares separately so they could distribute extra assets (accumulated as accrued fees or direct, not deposited, transfers) pro rata among depositors (shareholders). And Vault does implement this mechanism, but in a different way: it uses the beforeWithdraw function to calculate the amounts share holders are eligible for after an epoch is finalized. This is the same logic that's implemented by the shares-to-assets and assets-to-shares functions from EIP-4626.

Recommended Mitigation Steps

Since this is a non-critical issue, no changes are needed to be made. But the beforeWithdraw function is extra code that adds complexity and that implements what's already there–pro rata distribution of assets to shareholders, which is provided by the ERC4626 contract.

[N-02] SafeMath is not required in Solidity >=0.8.0

Targets

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

Description

Solidity 0.8.0 introduced the native check for underflows/overflows.