Revert Lend - JecikPo's results

Lines of code

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

Vulnerability details

Impact

The price calculation at the V3Oracle.sol will revert upon reaching certain level when referenceToken is used with high decimal value (e. g. 18). The revert (specifically happening when calling getValue()) would make the Chainlink price feed useless. Yet the TWAP price source would still be available. The protocol team would have to disable Chainlink and rely exclusively on the TWAP source reducing security of the pricing. The issue could manifest itself after certain amount of time once the project is already live and only when price returned by the feed reaches certain point.

Proof of Concept

The following code line has an issue:

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

When referenceTokenDecimals is 18 chainlinkPriceX96 is higher than some threshold between 18 and 19 (in Q96 notation), it will cause arithmetic overflow.

Tools Used

Manual review.

Recommended Mitigation Steps

Instead of calculating the price this way:

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

It could be done the following way as per Chainlink's recommendation:

if (referenceTokenDecimals > feedConfig.tokenDecimals)
            chainlinkPriceX96 = (10 ** referenceTokenDecimals - feedConfig.tokenDecimals) * chainlinkPriceX96 * Q96 
            / chainlinkReferencePriceX96;
        else if (referenceTokenDecimals < feedConfig.tokenDecimals)
            chainlinkPriceX96 = chainlinkPriceX96 * Q96 / chainlinkReferencePriceX96 
            / (10 ** feedConfig.tokenDecimals - referenceTokenDecimals);
        else 
            chainlinkPriceX96 = chainlinkPriceX96 * Q96 / chainlinkReferencePriceX96;

https://docs.chain.link/data-feeds/using-data-feeds#getting-a-different-price-denomination

Assessed type

Decimal

Lines of code

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

Vulnerability details

Impact

An attacker by front-running any liquidate() call can prevent successful liquidation by making it revert. By preventing liquidations the gains of the protocol can be diminished using relatively low cost, especially during volatile price movements.

Proof of Concept

The problem lies in the following condition inside the liquidate() function in the V3Vault.sol:

uint256 debtShares = loans[params.tokenId].debtShares;
        if (debtShares != params.debtShares) {
            revert DebtChanged();
        }

The liquidate() function requires that the caller puts the exact amount of debt that he intends to liquidate, that amount must equal the actual debt of a loan. An attacker can front-run the liquidate() transaction with a repay() and repay minimum amount of assets so that at least debtShares is reduced by 1.

A test to see the behaviour:

function testLiquidationFrontRun() external {
        address FRONT_RUNNER = 0xF977814e90Da44bfa03b6295a0616A897441bAcA;

        vm.prank(WHALE_ACCOUNT);
        USDC.transfer(FRONT_RUNNER, 1000);

        // approve front runner to vault for repay()
        vm.prank(FRONT_RUNNER);
        USDC.approve(address(vault), 100);

        _setupBasicLoan(true);

        // wait 7 days for interest to grow
        vm.warp(block.timestamp + 7 days);

        (,,, uint256 liquidationCost,) = vault.loanInfo(TEST_NFT);
        (uint256 debtShares) = vault.loans(TEST_NFT);
        vm.prank(WHALE_ACCOUNT);
        USDC.approve(address(vault), liquidationCost);

        vm.prank(FRONT_RUNNER);
        vault.repay(TEST_NFT, 1, true);

        vm.expectRevert(IErrors.DebtChanged.selector);
        vm.prank(WHALE_ACCOUNT);
        vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Recommendation is to remove the above condition completely and the debtShares from LiquidateParams. liquidation should always liquidate the entire available debt.

Assessed type

DoS

[L-01] repay() function can be front-run with repaying dust

Anyone can repay any loan. So when an original loan owner wishes to repay the whole debt by specifying the attributes of the repay(), the transaction could be front-run by an attacker where he repays the exact amount so that the loan.debtShares decrements by 1. This way the debt owner's transaction gets reverted due to the below condition.

File: src/V3Vault.sol

973: if (shares > currentShares) {
            revert RepayExceedsDebt();
        }

Recommendation:

The fix should be the following: If the debt/loan owner specifies too high amount, and the above condition is hit, the shares/assets values should be recalculated, like this:

973: if (shares > currentShares) {
         shares = currentShares
         assets = _convertToAssets(shares, newDebtExchangeRateX96, Math.Rounding.Up);
     }

This way would protect the loan/debt owners from unnecessarily repeated transaction and hence save them gas.

GitHub : 973

[L-02] Uniswap positions minted to V3Vault are stuck

Using the NonfungiblePositionManager directly a user could potentially mint a position directly to V3Vault by having V3Vault as the receiver when calling NonfungiblePositionManager.mint().

Since NonfungiblePositionManager doesn’t use safeMint() this would just transfer the token and liquidity to V3Vault without calling V3Vault.onERC721Received(), thus the liquidity would be lost.

Recommendation

Consider adding a rescue function callable by owner that can transfer any accidentally minted tokens out of the contract. This could check that V3Vault is the owner of the token but has loans[tokenId].owner == address(0) to prevent misuse.