Revert Lend - thank_you's results

Lines of code

https://github.com/Uniswap/permit2/blob/main/src/SignatureTransfer.sol?plain=1#L51-L68 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol?plain=1#L718-L725

Vulnerability details

Impact

When a user liquidates a loan in the Vault and V3Vault.liquidate() is called with permit data, the liquidator can set the permitted token to a token other than the Vault's asset. When this happens, the permitted token is used as liquidation payment. A malicious user could create a random ERC20 token and then liquidate a loan with this new ERC20 token for free.

Proof of Concept

Let's first discuss the Permit2 _permitTransferFrom function which shows how tokens are transferred:

function _permitTransferFrom(
    PermitTransferFrom memory permit,
    SignatureTransferDetails calldata transferDetails,
    address owner,
    bytes32 dataHash,
    bytes calldata signature
) private {
    uint256 requestedAmount = transferDetails.requestedAmount;

    if (block.timestamp > permit.deadline) revert SignatureExpired(permit.deadline);
    if (requestedAmount > permit.permitted.amount) revert InvalidAmount(permit.permitted.amount);

    _useUnorderedNonce(owner, permit.nonce);

    signature.verify(_hashTypedData(dataHash), owner);

    // AUDIT: below we see how the PermitTransferFrom which is created by the depositor defines which token the transfer occurs
    ERC20(permit.permitted.token).safeTransferFrom(owner, transferDetails.to, requestedAmount);
}

As you can see above, the PermitTransferFrom structure defines which tokens are pulled by calling permit.permitted.token.

With that in mind, reviewing the _repay function contains the following code related to allowing permitted token transfers:

(ISignatureTransfer.PermitTransferFrom memory permit, bytes memory signature) =
    abi.decode(params.permitData, (ISignatureTransfer.PermitTransferFrom, bytes));

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

As you can see, there are no checks that the permit structure has the permit.permitted.token set to the vault asset.

Tools Used

Manual inspection.

Recommended Mitigation Steps

The protocol should add a check in V3Vault._repay() that confirms the permit.permitted.token is the Vault's address:

(ISignatureTransfer.PermitTransferFrom memory permit, bytes memory signature) =
    abi.decode(params.permitData, (ISignatureTransfer.PermitTransferFrom, bytes));

// AUDIT: check here to make sure that the permitted.token is the Vault's asset 
if (permit.permitted.token != asset) {
    // AUDIT: custom error that needs to be set
    revert WrongVaultAsset();
}

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

Assessed type

Invalid Validation

Lines of code

https://github.com/Uniswap/permit2/blob/main/src/SignatureTransfer.sol?plain=1#L51-L68 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol?plain=1#L978-L984

Vulnerability details

Impact

When a user repays their loan to the Vault and V3Vault.repay() is called with permit data, the repayer can set the permitted token to a token other than the Vault's asset. When this happens, the permitted token is used as repayment. A malicious user could create a random ERC20 token and then repay this new ERC20 token into the vault and clear their loan debt for free.

Proof of Concept

Let's first discuss the Permit2 _permitTransferFrom function which shows how tokens are transferred:

function _permitTransferFrom(
    PermitTransferFrom memory permit,
    SignatureTransferDetails calldata transferDetails,
    address owner,
    bytes32 dataHash,
    bytes calldata signature
) private {
    uint256 requestedAmount = transferDetails.requestedAmount;

    if (block.timestamp > permit.deadline) revert SignatureExpired(permit.deadline);
    if (requestedAmount > permit.permitted.amount) revert InvalidAmount(permit.permitted.amount);

    _useUnorderedNonce(owner, permit.nonce);

    signature.verify(_hashTypedData(dataHash), owner);

    // AUDIT: below we see how the PermitTransferFrom which is created by the depositor defines which token the transfer occurs
    ERC20(permit.permitted.token).safeTransferFrom(owner, transferDetails.to, requestedAmount);
}

As you can see above, the PermitTransferFrom structure defines which tokens are pulled by calling permit.permitted.token.

With that in mind, reviewing the _repay function contains the following code related to allowing permitted token transfers:

(ISignatureTransfer.PermitTransferFrom memory permit, bytes memory signature) =
    abi.decode(permitData, (ISignatureTransfer.PermitTransferFrom, bytes));
permit2.permitTransferFrom(
    permit, ISignatureTransfer.SignatureTransferDetails(address(this), assets), msg.sender, signature
);

As you can see, there are no checks that the permit structure has the permit.permitted.token set to the vault asset.

Tools Used

Manual inspection.

Recommended Mitigation Steps

The protocol should add a check in V3Vault._repay() that confirms the permit.permitted.token is the Vault's address:

(ISignatureTransfer.PermitTransferFrom memory permit, bytes memory signature) =
    abi.decode(permitData, (ISignatureTransfer.PermitTransferFrom, bytes));

// AUDIT: check here to make sure that the permitted.token is the Vault's asset 
if (permit.permitted.token != asset) {
    // AUDIT: custom error that needs to be set
    revert WrongVaultAsset();
}

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

Assessed type

Invalid Validation

Lines of code

https://github.com/Uniswap/permit2/blob/main/src/SignatureTransfer.sol?plain=1#L51-L68 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol?plain=1#L893-L899

Vulnerability details

Impact

When a user deposits assets into the Vault and V3Vault._deposit() is called, the depositor can set the permitted token to a token other than the Vault's asset. When this happens, the permitted token is deposited and shares are minted to the depositor. A malicious user could create a random ERC20 token and then deposit this new ERC20 token into the vault and receive shares for free.

Proof of Concept

Let's first discuss the Permit2 _permitTransferFrom function which shows how tokens are transferred:

function _permitTransferFrom(
    PermitTransferFrom memory permit,
    SignatureTransferDetails calldata transferDetails,
    address owner,
    bytes32 dataHash,
    bytes calldata signature
) private {
    uint256 requestedAmount = transferDetails.requestedAmount;

    if (block.timestamp > permit.deadline) revert SignatureExpired(permit.deadline);
    if (requestedAmount > permit.permitted.amount) revert InvalidAmount(permit.permitted.amount);

    _useUnorderedNonce(owner, permit.nonce);

    signature.verify(_hashTypedData(dataHash), owner);

    // AUDIT: below we see how the PermitTransferFrom which is created by the depositor defines which token the transfer occurs
    ERC20(permit.permitted.token).safeTransferFrom(owner, transferDetails.to, requestedAmount);
}

As you can see above, the PermitTransferFrom structure defines which tokens are pulled by calling permit.permitted.token.

With that in mind, reviewing the _deposit function contains the following code related to allowing permitted token transfers:

(ISignatureTransfer.PermitTransferFrom memory permit, bytes memory signature) =
    abi.decode(permitData, (ISignatureTransfer.PermitTransferFrom, bytes));
permit2.permitTransferFrom(
    permit, ISignatureTransfer.SignatureTransferDetails(address(this), assets), msg.sender, signature
);

As you can see, there are no checks that the permit structure has the permit.permitted.token set to the vault asset.

Below is the following forge test that shows how a malicious user can set the permitted token to be an address other than the Vault's asset:

function testDepositPermit2Exploit() external {
    uint256 amount = 1000000;
    uint256 privateKey = 123;
    address addr = vm.addr(privateKey);

    // give coins
    vm.deal(addr, 1 ether);
    vm.prank(WHALE_ACCOUNT);
    // DAI.transfer(addr, amount * 2);
    deal(address(DAI), addr, amount * 2);

    vm.prank(addr);
    DAI.approve(PERMIT2, type(uint256).max);

    ISignatureTransfer.PermitTransferFrom memory tf = ISignatureTransfer.PermitTransferFrom(
        ISignatureTransfer.TokenPermissions(address(DAI), 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.lendInfo(addr), 1000000);
    assertEq(vault.asset() == address(USDC), true);
}

Tools Used

Manual inspection.

Recommended Mitigation Steps

The protocol should add a check in V3Vault._deposit() that confirms the permit.permitted.token is the Vault's address:

(ISignatureTransfer.PermitTransferFrom memory permit, bytes memory signature) =
    abi.decode(permitData, (ISignatureTransfer.PermitTransferFrom, bytes));

// AUDIT: check here to make sure that the permitted.token is the Vault's asset 
if (permit.permitted.token != asset) {
    // AUDIT: custom error that needs to be set
    revert WrongVaultAsset();
}

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/main/src/V3Vault.sol?plain=1#L1167-L1195 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol?plain=1#L837-L840

Vulnerability details

Impact

When the vault owner calls V3Vault.setReserveFactor(), the function updates the reserve factor for the Vault. Unfortunately, if the global interest is not updated before the reserve factor is updated, the updated reserve factor will be retroactively applied in the exchange rate formula starting at the last update. This leads to unexpected lending rate changes causing lenders to receive unexpected more or less favorable lending exchange rate depending on what the updated reserve factor value is.

Proof of Concept

The lending rate formula is calculated _calculateGlobalInterest() and the formula can be defined as:

(uint256 borrowRateX96, uint256 supplyRateX96) = interestRateModel.getRatesPerSecondX96(available, debt);

supplyRateX96 = supplyRateX96.mulDiv(Q32 - reserveFactorX32, Q32);

// always growing or equal
uint256 lastRateUpdate = lastExchangeRateUpdate;

if (lastRateUpdate > 0) {
    newDebtExchangeRateX96 = oldDebtExchangeRateX96 + oldDebtExchangeRateX96 * (block.timestamp - lastRateUpdate) * borrowRateX96 / Q96;
    newLendExchangeRateX96 = oldLendExchangeRateX96 + oldLendExchangeRateX96 * (block.timestamp - lastRateUpdate) * supplyRateX96 / Q96;
} else {
    newDebtExchangeRateX96 = oldDebtExchangeRateX96;
    newLendExchangeRateX96 = oldLendExchangeRateX96;
}

In the formula above, the supply rate is modified before being used to calculate the new lending rate as supply rate * reserve factor. This modified supply rate is then used to determine how much of a jump should occur in the lending rate via newLendExchangeRateX96 = oldLendExchangeRateX96 + oldLendExchangeRateX96 * (block.timestamp - lastRateUpdate) * supplyRateX96 / Q96. The larger the modified supply rate, the larger the jump is. The smaller the modified supply rate, the smaller the jump is.

By not updating the lending rate before updating the reserve factor, the updated reserve factor will retroactively be applied to the past artificially influencing the lending rate.

To best visualize this, let's look at the forge test below which shows two scenarios, one where the interest is updated before the reserve factor is updated and one where it's not. Then we can compare the different lending rate values and see how by not updating the exchange rates before updating the reserve factor, the lending rate is impacted.

RESULTS FROM FORGE TESTS 

with interest rate update occurring after reserve factor update:

- starting lendExchangeRateX96:  79243018781103204090820932736
- after reserve update lendExchangeRateX96:  79240047527736122590199028736

with interest rate update occurring before reserve factor update:

- starting lendExchangeRateX96:  79243018781103204090820932736
- after reserve update lendExchangeRateX96:  79243018781103204090820932736

function testLendingRateReserveFactorBugWithoutInterestUpdate() external {
    vault.setLimits(1000000, 1000e18, 1000e18, 1000e18, 1000e18);

    // set up basic vault settings
    _deposit(10000000, WHALE_ACCOUNT);
    _setupBasicLoan(true);
    vm.warp(block.timestamp + 7 days);

    (,,,,,,uint lendExchangeRateX96) = vault.vaultInfo();
    console.log("old lendExchangeRateX96: ", lendExchangeRateX96);

    vm.prank(vault.owner());

    vault.setReserveFactor(uint32(Q32 / 5)); // 20% reserve factor

    // AUDIT: Calling setLimits updates the exchange rate
    vault.setLimits(1000000, 1000e18, 1000e18, 1000e18, 1000e18);


    (,,,,,,lendExchangeRateX96) = vault.vaultInfo();
    console.log("new lendExchangeRateX96: ", lendExchangeRateX96);
}

function testLendingRateReserveFactorBugWithInterestUpdate() external {
    vault.setLimits(1000000, 1000e18, 1000e18, 1000e18, 1000e18);

    // set up basic vault settings
    _deposit(10000000, WHALE_ACCOUNT);
    _setupBasicLoan(true);
    vm.warp(block.timestamp + 7 days);

    (,,,,,,uint lendExchangeRateX96) = vault.vaultInfo();
    console.log("old lendExchangeRateX96: ", lendExchangeRateX96);

    vm.prank(vault.owner());

    // AUDIT: Calling setLimits updates the exchange rate
    vault.setLimits(1000000, 1000e18, 1000e18, 1000e18, 1000e18);
    vault.setReserveFactor(uint32(Q32 / 5)); // 20% reserve factor

    (,,,,,,lendExchangeRateX96) = vault.vaultInfo();
    console.log("new lendExchangeRateX96: ", lendExchangeRateX96);
}

As you can see, by not updating the interest rate before updating the reserve factor, the lending rate will be impacted unfairly.

Tools Used

Manual inspection.

Recommended Mitigation Steps

Add _updateGlobalInterest() to the V3Vault.setReserveFactor() function before the reserve factor is updated. This ensures that the lending rate will not be artificially impacted and the updated reserve factor is not retroactively applied to the past:

function setReserveFactor(uint32 _reserveFactorX32) external onlyOwner {
    _updateGlobalInterest();
    reserveFactorX32 = _reserveFactorX32;
}

Assessed type

Math

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol?plain=1#L735 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol?plain=1#L1032-L1073

Vulnerability details

Impact

When Vault.liquidate() is called, the protocol allows users to provide a LiquidateParams struct as the param. Within the LiquidateParams struct is the recipient attribute which determines who receives the liquidation reward. Unfortunately, the liquidate() function sends the liquidation reward to the msg.sender (the Vault.liquidate() caller). This breaks an API expectation for liquidators who expect the liquidation reward to be delivered to the recipient. If the liquidator is a contract, those rewards may be permanently stuck in the contract and the recipient will never receive the rewards.

Proof of Concept

Let's look at the Vault.liquidate() function which shows the vulnerability:

function liquidate(LiquidateParams calldata params) external override returns (uint256 amount0, uint256 amount1) {
    ...
    // send promised collateral tokens to liquidator
    // AUDIT: note that msg.sender is passed in as the 5th-arg
    (amount0, amount1) =
        _sendPositionValue(params.tokenId, state.liquidationValue, state.fullValue, state.feeValue, msg.sender);
    ...
}

function _sendPositionValue(
    uint256 tokenId,
    uint256 liquidationValue,
    uint256 fullValue,
    uint256 feeValue,
    address recipient // AUDIT: note that this is set as msg.sender and NOT params.recipient in the liquidate()
) internal returns (uint256 amount0, uint256 amount1) {
    uint128 liquidity;
    uint128 fees0;
    uint128 fees1;

    // if full position is liquidated - no analysis needed
    if (liquidationValue == fullValue) {
        (,,,,,,, liquidity,,,,) = nonfungiblePositionManager.positions(tokenId);
        fees0 = type(uint128).max;
        fees1 = type(uint128).max;
    } else {
        (,,, liquidity,,, fees0, fees1) = oracle.getPositionBreakdown(tokenId);

        // only take needed fees
        if (liquidationValue < feeValue) {
            liquidity = 0;
            fees0 = uint128(liquidationValue * fees0 / feeValue);
            fees1 = uint128(liquidationValue * fees1 / feeValue);
        } else {
            // take all fees and needed liquidity
            fees0 = type(uint128).max;
            fees1 = type(uint128).max;
            liquidity = uint128((liquidationValue - feeValue) * liquidity / (fullValue - feeValue));
        }
    }

    if (liquidity > 0) {
        nonfungiblePositionManager.decreaseLiquidity(
            INonfungiblePositionManager.DecreaseLiquidityParams(tokenId, liquidity, 0, 0, block.timestamp)
        );
    }

    (amount0, amount1) = nonfungiblePositionManager.collect(
        // AUDIT: msg.sender ends up getting the reward
        INonfungiblePositionManager.CollectParams(tokenId, recipient, fees0, fees1)
    );
}

As you can see above, Vault.liquidate() calls _sendPositionValue() which sends the liquidation reward to the recipient. In the liquidate function, by setting the _sendPositionValue function's 5th argument to msg.sender, the Vault.liquidate() caller will always receive the liquidation rewards.

Tools Used

Manual inspection.

Recommended Mitigation Steps

Revert should pass the params.recipient as the 5th argument when calling _sendPositionValue:

_sendPositionValue(params.tokenId, state.liquidationValue, state.fullValue, state.feeValue, params.recipient);

Assessed type

Payable

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol?plain=1#L877-L917 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol?plain=1#L1281-L1295

Vulnerability details

Impact

When a user calls V3Vault.mint() or V3Vault.deposit(), the user is unable to provide slippage arguments to ensure that they receive at least X amount of lending shares. If their transaction is delayed before it's executed or is frontrun, the user may receive less lending shares than expected.

Proof of Concept

Below is the functionality for _deposit() which calculates how many lending shares to mint and contains no slippage checks:

function _deposit(address receiver, uint256 amount, bool isShare, bytes memory permitData)
    internal
    returns (uint256 assets, uint256 shares)
{
    (, uint256 newLendExchangeRateX96) = _updateGlobalInterest();

    _resetDailyLendIncreaseLimit(newLendExchangeRateX96, false);

    if (isShare) {
        shares = amount;
        assets = _convertToAssets(shares, newLendExchangeRateX96, Math.Rounding.Up);
    } else {
        assets = amount;
        shares = _convertToShares(assets, newLendExchangeRateX96, Math.Rounding.Down);
    }

    if (permitData.length > 0) {
        (ISignatureTransfer.PermitTransferFrom memory permit, bytes memory signature) =
            abi.decode(permitData, (ISignatureTransfer.PermitTransferFrom, bytes));
        permit2.permitTransferFrom(
            permit, ISignatureTransfer.SignatureTransferDetails(address(this), assets), msg.sender, signature
        );
    } else {
        // fails if not enough token approved
        SafeERC20.safeTransferFrom(IERC20(asset), msg.sender, address(this), assets);
    }

    _mint(receiver, shares);

    if (totalSupply() > globalLendLimit) {
        revert GlobalLendLimit();
    }

    if (assets > dailyLendIncreaseLimitLeft) {
        revert DailyLendIncreaseLimit();
    } else {
        dailyLendIncreaseLimitLeft -= assets;
    }

    emit Deposit(msg.sender, receiver, assets, shares);
}

To determine the amount of shares to mint for the user in _deposit(), the protocol utilizes the newLendExchangeRateX96 to calculate how many shares to mint:

if (isShare) {
    shares = amount;
    assets = _convertToAssets(shares, newLendExchangeRateX96, Math.Rounding.Up);
} else {
    assets = amount;
    shares = _convertToShares(assets, newLendExchangeRateX96, Math.Rounding.Down);
}

function _convertToShares(uint256 amount, uint256 exchangeRateX96, Math.Rounding rounding)
    internal
    pure
    returns (uint256)
{
    return amount.mulDiv(Q96, exchangeRateX96, rounding);
}

function _convertToAssets(uint256 shares, uint256 exchangeRateX96, Math.Rounding rounding)
    internal
    pure
    returns (uint256)
{
    return shares.mulDiv(exchangeRateX96, Q96, rounding);
}

Since the newLendExchangeRateX96 will update over time (see _calculateGlobalInterest() code snippet below), the amount of shares minted will decrease over time. This leads to a user experiencing slippage.

// always growing or equal
uint256 lastRateUpdate = lastExchangeRateUpdate;

if (lastRateUpdate > 0) {
    newDebtExchangeRateX96 = oldDebtExchangeRateX96
        + oldDebtExchangeRateX96 * (block.timestamp - lastRateUpdate) * borrowRateX96 / Q96;
    // lend exchange rate changes over time
    newLendExchangeRateX96 = oldLendExchangeRateX96
        + oldLendExchangeRateX96 * (block.timestamp - lastRateUpdate) * supplyRateX96 / Q96;
} else {
    newDebtExchangeRateX96 = oldDebtExchangeRateX96;
    newLendExchangeRateX96 = oldLendExchangeRateX96;
}

Tools Used

Manual inspection.

Recommended Mitigation Steps

Allow the user to provide minimum slippage checks when calling mint() (slippage check against assets required to create X amount of shares) or deposit() (slippage checks against shares minted). This will ensure that the user receives at least X amount of lending shares when calling mint() or deposit().

Assessed type

Timing