Revert Lend - SpicyMeatball's results

Lines of code

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

Vulnerability details

Impact

Can't liquidate a loan if the loan owner doesn't want to be liquidated.

Proof of Concept

At the end of the liquidate function, the protocol transfers disarmed NFT to the loan owner. https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L744

    // cleans up loan when it is closed because of replacement, repayment or liquidation
    // send the position in its current state to owner
    function _cleanupLoan(uint256 tokenId, uint256 debtExchangeRateX96, uint256 lendExchangeRateX96, address owner)
        internal
    {
        _removeTokenFromOwner(owner, tokenId);
        _updateAndCheckCollateral(tokenId, debtExchangeRateX96, lendExchangeRateX96, loans[tokenId].debtShares, 0);
        delete loans[tokenId];
>>      nonfungiblePositionManager.safeTransferFrom(address(this), owner, tokenId);
        emit Remove(tokenId, owner);
    }

If the loan owner is a contract, it can revert the transaction inside the onERC721Received callback.

Tools Used

Manual review

Recommended Mitigation Steps

Let liquidated users to claim their NFTs instead of transferring them.

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Oracles where reference token has 18 decimals will not work in chainlink modes.

Proof of Concept

Let's check V3Oracle's _getReferenceTokenPriceX96 function.

    function _getReferenceTokenPriceX96(address token, uint256 cachedChainlinkReferencePriceX96)
        internal
        view
        returns (uint256 priceX96, uint256 chainlinkReferencePriceX96)
    {
        ...
        if (usesChainlink) {
            uint256 chainlinkPriceX96 = _getChainlinkPriceX96(token);
            chainlinkReferencePriceX96 = cachedChainlinkReferencePriceX96 == 0
                ? _getChainlinkPriceX96(referenceToken)
                : cachedChainlinkReferencePriceX96;

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

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Oracle.sol#L304 Line 304 will most likely overflow if referenceTokenDecimals = 18, for example if the referenceToken is DAI and the token is WETH, the dividend will be 10 ** 18 * 3500 * 2 ** 96 * 2 ** 96 = 2,2 * 10 ** 79 which is greater than uint256 can handle.

Tools Used

Manual review

Recommended Mitigation Steps

Use FullMath.mulDiv function to prevent intermediate value overflows.

Assessed type

Oracle

Lines of code

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

Vulnerability details

Impact

The user can frontrun liquidation transaction with one wei loan repayment, making it to revert with debtChanged error.

Proof of Concept

When the liquidator calls liquidate function he specifies loan debtShares in parameters,

    function liquidate(LiquidateParams calldata params) external override returns (uint256 amount0, uint256 amount1) {
        // liquidation is not allowed during transformer mode
        if (transformedTokenId > 0) {
            revert TransformNotAllowed();
        }

        LiquidateState memory state;

        (state.newDebtExchangeRateX96, state.newLendExchangeRateX96) = _updateGlobalInterest();

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

this value is then compared to actual debtShares, if these values are not equal the liquidation fails. This allows the loan owner to evade liquidation by repaying dust amounts just before the liquidation call.

V3Vault.t.sol _testLiquidation https://github.com/code-423n4/2024-03-revert-lend/blob/main/test/integration/V3Vault.t.sol#L731-L747

    function _testLiquidation(LiquidationType lType) internal {
        ...
     
        vm.prank(WHALE_ACCOUNT);
        USDC.approve(address(vault), liquidationCost - 1);

        (uint256 debtShares) = vault.loans(TEST_NFT);

        vm.prank(WHALE_ACCOUNT);
        vm.expectRevert("ERC20: transfer amount exceeds allowance");
        vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));

        vm.prank(WHALE_ACCOUNT);
        USDC.approve(address(vault), liquidationCost);

        uint256 daiBalance = DAI.balanceOf(WHALE_ACCOUNT);
        uint256 usdcBalance = USDC.balanceOf(WHALE_ACCOUNT);

+       vm.startPrank(TEST_NFT_ACCOUNT);
+       USDC.approve(address(vault), 1);
+       vault.repay(TEST_NFT, 1, true);
+       vm.stopPrank();

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

Tools Used

Foundry

Recommended Mitigation Steps

Consider adding a time cooldown between repayments.

Assessed type

DoS