Risk of reentrancy `onERC721Received` function to manipulate collateral token configs shares

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L454-L473 https://github.com/code-423n4/2024-03-revert-lend/blob/main/src/V3Vault.sol#L1223-L1241

Vulnerability details

Issue Description

The onERC721Received function is invoked whenever the vault contract receives a Uniswap V3 position ERC721 token. This can happen either when an owner creates a new position or when a transformation occurs.

For this issue, we'll focus on the second case, specifically when a position is going through a transformation, which creates a new position token. In such a case, we have tokenId != oldTokenId, and the else block is run, as shown below:

function onERC721Received(address, address from, uint256 tokenId, bytes calldata data)
    external
    override
    returns (bytes4)
{
    ...

    if {
        ...
    } else {
        uint256 oldTokenId = transformedTokenId;

        // if in transform mode - and a new position is sent - current position is replaced and returned
        if (tokenId != oldTokenId) {
            address owner = tokenOwner[oldTokenId];

            // set transformed token to new one
            transformedTokenId = tokenId;

            // copy debt to new token
            loans[tokenId] = Loan(loans[oldTokenId].debtShares);

            _addTokenToOwner(owner, tokenId);
            emit Add(tokenId, owner, oldTokenId);

            // clears data of old loan
            _cleanupLoan(oldTokenId, debtExchangeRateX96, lendExchangeRateX96, owner);

            //@audit can reenter with onERC721Received and call repay or borrow to call _updateAndCheckCollateral twice and manipulate collateral token configs

            // sets data of new loan
            _updateAndCheckCollateral(
                tokenId, debtExchangeRateX96, lendExchangeRateX96, 0, loans[tokenId].debtShares
            );
        }
    }

    return IERC721Receiver.onERC721Received.selector;
}

We should note that the _cleanupLoan function does return the old position token to the 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);
}

The issue that can occur is that the _cleanupLoan is invoked before the _updateAndCheckCollateral call. So, a malicious owner can use the onERC721Received callback when receiving the old token to call the borrow function, which makes changes to loans[tokenId].debtShares and calls _updateAndCheckCollateral. When the call resumes, the V3Vault.onERC721Received function will call _updateAndCheckCollateral again, resulting in incorrect accounting of internal token configs debt shares (tokenConfigs[token0].totalDebtShares & tokenConfigs[token1].totalDebtShares) and potentially impacting the vault borrowing process negatively.

Proof of Concept

Let's use the following scenario to demonstrate the issue:

Before starting, we suppose the following states:

• tokenConfigs[token0].totalDebtShares = 10000
• tokenConfigs[token1].totalDebtShares = 15000

1. Bob has previously deposited a UniswapV3 position (which uses token0 and token1) with tokenId = 12 and borrowed loans[tokenId = 12].debtShares = 1000 debt shares.

2. Bob calls the transform function to change the range of his position using the AutoRange transformer, which mints a new ERC721 token tokenId = 20 for the newly arranged position and sends it to the vault.

3. Upon receiving the new token, the V3Vault.onERC721Received function is triggered. As we're in transformation mode and the token ID is different, the second else block above will be executed.

4. V3Vault.onERC721Received will copy loan debt shares to the new token, so we'll have loans[tokenId = 20].debtShares = 1000.

5. Then V3Vault.onERC721Received will invoke the _cleanupLoan function to clear the data of the old loan and transfer the old position token tokenId = 12 back to Bob.

   5.1. _cleanupLoan will also call _updateAndCheckCollateral function to change oldShares = 1000 --> newShares = 0 (remove old token shares), resulting in:

   • tokenConfigs[token0].totalDebtShares = 10000 - 1000 = 9000
   • tokenConfigs[token1].totalDebtShares = 15000 - 1000 = 14000

6. Bob, upon receiving the old position token, will also use the ERC721 onERC721Received callback to call the borrow function. He will borrow 200 debt shares against his new position token tokenId = 20.

   6.1. The borrow function will update the token debt shares from loans[tokenId = 20].debtShares = 1000 to: loans[tokenId = 20].debtShares = 1000 + 200 = 1200 (assuming the position is healthy).

   6.2. The borrow function will also invoke the _updateAndCheckCollateral function to change oldShares = 1000 --> newShares = 1200 for tokenId = 20, resulting in:

   • tokenConfigs[token0].totalDebtShares = 9000 + 200 = 9200
   • tokenConfigs[token1].totalDebtShares = 14000 + 200 = 14200

7. Bob's borrow call ends, and the V3Vault.onERC721Received call resumes. _updateAndCheckCollateral gets called again, changing oldShares = 0 --> newShares = 1200 (as the borrow call changed the token debt shares), resulting in:

   • tokenConfigs[token0].totalDebtShares = 9200 + 1200 = 10400
   • tokenConfigs[token1].totalDebtShares = 14200 + 1200 = 15400

Now, let's assess what Bob managed to achieve by taking a normal/honest transformation process (without using the onERC721Received callback) and then a borrow operation scenario:

• Normally, when V3Vault.onERC721Received is called, it shouldn't change the internal token configs debt shares (tokenConfigs[token0].totalDebtShares & tokenConfigs[token1].totalDebtShares). After a normal V3Vault.onERC721Received, we should still have:

  • tokenConfigs[token0].totalDebtShares = 10000
  • tokenConfigs[token1].totalDebtShares = 15000

• Then, when Bob borrows 200 debt shares against the new token, we should get:

  • tokenConfigs[token0].totalDebtShares = 10000 + 200 = 10200
  • tokenConfigs[token1].totalDebtShares = 15000 + 200 = 15200

We observe that by using the onERC721Received callback, Bob managed to increase the internal token configs debt shares (tokenConfigs[token0].totalDebtShares & tokenConfigs[token1].totalDebtShares) by 200 debt shares more than expected.

This means that Bob, by using this attack, has manipulated the internal token configs debt shares, making the vault believe it has 200 additional debt shares. Bob can repeat this attack multiple times until he approaches the limit represented by collateralValueLimitFactorX32 and collateralValueLimitFactorX32 multiplied by the amount of asset lent as shown below:

uint256 lentAssets = _convertToAssets(
    totalSupply(),
    lendExchangeRateX96,
    Math.Rounding.Up
);
uint256 collateralValueLimitFactorX32 = tokenConfigs[token0]
    .collateralValueLimitFactorX32;
if (
    collateralValueLimitFactorX32 < type(uint32).max &&
    _convertToAssets(
        tokenConfigs[token0].totalDebtShares,
        debtExchangeRateX96,
        Math.Rounding.Up
    ) >
    (lentAssets * collateralValueLimitFactorX32) / Q32
) {
    revert CollateralValueLimit();
}
collateralValueLimitFactorX32 = tokenConfigs[token1]
    .collateralValueLimitFactorX32;
if (
    collateralValueLimitFactorX32 < type(uint32).max &&
    _convertToAssets(
        tokenConfigs[token1].totalDebtShares,
        debtExchangeRateX96,
        Math.Rounding.Up
    ) >
    (lentAssets * collateralValueLimitFactorX32) / Q32
) {
    revert CollateralValueLimit();
}

Then, when other borrowers try to call the borrow function, it will revert because _updateAndCheckCollateral will trigger the CollateralValueLimit error, thinking there is too much debt already. However, this is not the case, as the internal token configs debt shares have been manipulated (increased) by an attacker (Bob).

This attack is irreversible because there is no way to correct the internal token configs debt shares (tokenConfigs[token0].totalDebtShares & tokenConfigs[token1].totalDebtShares), and the vault will remain in that state, not allowing users to borrow, resulting in no interest being accrued and leading to financial losses for the lenders and the protocol.

Impact

A malicious attacker could use the AutoRange transformation process to manipulate the internal token configs debt shares, potentially resulting in:

• Fewer loans being allowed by the vault than expected.
• A complete denial-of-service (DOS) for all borrow operations.

Tools Used

Manual review, VS Code

Recommended Mitigation

The simplest way to address this issue is to ensure that the onERC721Received function follows the Correctness by Construction (CEI) pattern, as follows:

function onERC721Received(address, address from, uint256 tokenId, bytes calldata data)
    external
    override
    returns (bytes4)
{
    ...

    if {
        ...
    } else {
        uint256 oldTokenId = transformedTokenId;

        // if in transform mode - and a new position is sent - current position is replaced and returned
        if (tokenId != oldTokenId) {
            address owner = tokenOwner[oldTokenId];

            // set transformed token to new one
            transformedTokenId = tokenId;

            // copy debt to new token
            loans[tokenId] = Loan(loans[oldTokenId].debtShares);

            _addTokenToOwner(owner, tokenId);
            emit Add(tokenId, owner, oldTokenId);

--          // clears data of old loan
--          _cleanupLoan(oldTokenId, debtExchangeRateX96, lendExchangeRateX96, owner);

            // sets data of new loan
            _updateAndCheckCollateral(
                tokenId, debtExchangeRateX96, lendExchangeRateX96, 0, loans[tokenId].debtShares
            );

++          // clears data of old loan
++          _cleanupLoan(oldTokenId, debtExchangeRateX96, lendExchangeRateX96, owner);
        }
    }

    return IERC721Receiver.onERC721Received.selector;
}

Assessed type

Context

[c4-pre-sort]

0xEVom marked the issue as duplicate of #309

[c4-pre-sort]

0xEVom marked the issue as sufficient quality report

[c4-judge]

jhsagd76 marked the issue as satisfactory

[c4-judge]

jhsagd76 changed the severity to 2 (Med Risk)

[c4-judge]

jhsagd76 marked the issue as not a duplicate

[c4-judge]

jhsagd76 marked the issue as primary issue

[c4-judge]

jhsagd76 changed the severity to 3 (High Risk)

[c4-judge]

jhsagd76 marked the issue as selected for report

[kalinbas]

https://github.com/revert-finance/lend/pull/8 https://github.com/revert-finance/lend/pull/32