Y2k Finance contest - carrotsmuggler's results

Lines of code

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

Vulnerability details

Impact

Wrong maths for handling pricefeed decimals. This code will only work for pricefeeds of 8 decimals, any others give wrong/incorrect data. The maths used can be shown in three lines:

nowPrice = (price1 * 10000) / price2;
nowPrice = nowPrice * int256(10**(18 - priceFeed1.decimals()));
return nowPrice / 1000000;

Line1: adds 4 decimals Line2: adds (18 - d) decimals, (where d = pricefeed.decimals()) Line3: removes 6 decimals

Total: adds (16 - d) decimals

when d=8, the contract correctly returns an 8 decimal number. However, when d = 6, the function will return a 10 decimal number. This is further raised by (18-d = 12) decimals when checking for depeg event, leading to a 22 decimal number which is 4 orders of magnitude incorrect.

if d=18, (like usd-eth pricefeeds) contract fails / returns 0.

All chainlink contracts which give price in eth, operate with 18 decimals. So this can cripple the system if added later.

Proof of Concept

Running the test AssertTest.t.sol:testPegOracleMarketCreation and changing the line on

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/test/AssertTest.t.sol#L30

to

PegOracle pegOracle3 = new PegOracle(
            0xB1552C5e96B312d0Bf8b554186F846C40614a540,  //usd-eth contract address
            btcEthOracle
        );

gives this output

oracle3price1: 1085903802394919427                                                                                                                                                                               
oracle3price2: 13753840915281064000                                                                                                                                                                              
oracle3price1 / oracle3price2: 0

returning an oracle value of 0. Simulating with a mock price feed of 6 decimals gives results 4 orders of magnitude off.

Tools Used

Foundry, vs-code

Recommended Mitigation Steps

Since only the price ratio is calculated, there is no point in increasing the decimal by (18-d) in the second line. Proposed solution:

nowPrice = (price1 * 10000) / price2;
nowPrice = nowPrice * int256(10**(priceFeed1.decimals())) * 100;
return nowPrice / 1000000;

This returns results in d decimals, no matter the value of d.

Lines of code

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L148-L192 https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Controller.sol#L198-L248

Vulnerability details

Impact

Controller functions do not check for empty vaults. If either hedge or risk vault is empty for any market / id, the premium / risk value can be irretrievably lost. When a vault value is 0 and the counterparty vault's funds are moved in, the idFinalTVL is set to 0, making any withdrawal/recovery impossible essentially permanently burning those tokens. Such burn functions should be avoided.

Also, if either vault is empty, it implies that either hedgers are hedging against nothing, or riskers are risking with no premium. Since the protocol does not allow the users to withdraw their funds before the start of the hedging period, it highly discourages people from adding money to either vault if the counterparty vault is empty.

Proof of Concept

Run the test in

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/test/AssertTest.t.sol#L243-L263

but with hedge vault 0 (no deposit from ALICE & BOB). The Premium amount will be locked forever. Similarly, test testControllerDepeg with risk vault 0 (no deposit from CHAD & DEGEN).

Tools Used

Vs-code, Foundry

Recommended Mitigation Steps

Check if either vault (hedge/risk) is empty before triggering depeg/ endepoch. If any vault is 0, allow withdrawal of funds without triggering the inter-vault token transfers (basically a refund).

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

Low

1. Check epochBegin time when creating a new vault

When creating a new vault, epochBegin time should be checked against the block.timestamp. Otherwise can lead to the creation of unusable vaults. https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/src/Vault.sol#L307-L325

2. Withdrawal before epoch begins

Maybe allow the withdrawal of funds before the epoch begins to fix mistakes etc regarding deposit amounts. Current implementation only allows withdrawal after depeg event, or epochEnd. Can be ignored if this is the intended method of operation.

Non-Critical

1. Inconsistent usage of vault names

Some places address the vault as insr, some places address it as hedge. Advised to have consistent naming to prevent confusion.

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

2. Wrong maths in comment

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

Incorrect expression in comments. Should be // 0.5% = multiply by 5 then divide by 1000

3. Failing test in AssertTest.t.sol:testOwnerAuthorize

The test in

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/test/AssertTest.t.sol#L493-L528

fails since the controller is never called to end the epoch. Insert the line controller.triggerEndEpoch(1, endEpoch); before starting the withdrawal process.

4. Failing fuzz tests in FuzzTest.t.sol

2 Fuzz tests fail in FuzzTest.t.sol, both due to the same reason: an issue in the test function testFuzzDeposit().

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/test/FuzzTest.t.sol#L17-L61

The reason for failure is that the FixedPointMathLib.sol library can only handle uint128 numbers. It multiplies two numbers and stores them in z

https://github.com/code-423n4/2022-09-y2k-finance/blob/2175c044af98509261e4147edeb48e1036773771/lib/solmate/src/utils/FixedPointMathLib.sol#L41

This only works if the product fits in uint256, which can be guaranteed with uint128 numbers. The fuzz test sends a number larger than uint128 since it expects to test uint256, making the test revert. A simple fix is to put in an assume statement at the top, only accepting uint96. uint96 as an example, because we want the result of the number * MULTIPLIER to fit within a uint128. Add the assume as below

vm.assume(ethValue < type(uint96).max && ethValue > 1);

The second condition is needed otherwise it reverts if sent 0 ETH by design. The other failing fuzz test fails because it calls this function. So adding this one line will fix all tests in FuzzTest.t.sol.