Revert Lend - erosjohn's results

Lines of code

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

Details

V3Vault.sol is not EIP-4626 compliant, variation from the standard could break composability and result in revert. take the V3Vault.sol#maxWithdraw as a example：

    function maxWithdraw(address owner) external view override returns (uint256) {
        (, uint256 lendExchangeRateX96) = _calculateGlobalInterest();
        return _convertToAssets(balanceOf(owner), lendExchangeRateX96, Math.Rounding.Down);
    }

This function only calculates the result through balanceOf(owner) and lendExchangeRateX96, without considering whether there are enough underlying assets in the contract. If there are insufficient underlying assets in the contract, when the user calls withdraw(maxWithdraw(owner), owner, owner), it will cause revert, which does not comply with EIP4626.

The relevant specifications are as follows: MUST return the maximum amount of assets that could be transferred from owner through withdraw and not cause a revert, which MUST NOT be higher than the actual maximum that would be accepted (it should underestimate if necessary).

Impact

When the user calls withdraw with the return value of V3Vault.sol#maxWithdraw as an input parameter, it is possible to revert, which does not comply with EIP-4626.

Proof of Concept

Add the test code to V3Vault.t.sol and run forge test -vvv --mt testScenario_Poc.

    function testScenario_Poc() external {
        vault.setLimits(1000000, 15000000, 15000000, 15000000, 15000000);

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

        // Bob create a position
        vm.prank(TEST_NFT_ACCOUNT);
        NPM.approve(address(vault), TEST_NFT);
        vm.prank(TEST_NFT_ACCOUNT);
        vault.create(TEST_NFT, TEST_NFT_ACCOUNT);

        // Bob borrow 8 USDC
        vm.prank(TEST_NFT_ACCOUNT);
        vault.borrow(TEST_NFT, 8e6);

        vm.roll(block.number + 10);
        vm.warp(block.timestamp + 120);

        // Alice wants to withdraw assets at maximum value
        uint256 maximum = vault.maxWithdraw(WHALE_ACCOUNT);
        console.log("Alice wants to withdraw: ", maximum, "\n  But vault's USDC balance is:",USDC.balanceOf(address(vault)));
        vm.expectRevert(IErrors.InsufficientLiquidity.selector);
        vm.prank(WHALE_ACCOUNT);
        vault.withdraw(maximum, WHALE_ACCOUNT, WHALE_ACCOUNT);
    }

The output is:

Alice wants to withdraw: 10000001
But vault's USDC balance is: 2000000

Tools Used

Manual Code Review

Recommended Mitigation Steps

In maxWithdraw, compare the original return value with the balance of the vault's underlying assets, and return the smaller value.

Assessed type

ERC4626

Lines of code

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

Vulnerability details

Details

In V3Vault.sol#liquidate, it will check whether loans[params.tokenId].debtShares and params.debtShares are equal. If not, it will cause revert. This check is too strict and may result in DoS: V3Vault.sol#L695-L698

        uint256 debtShares = loans[params.tokenId].debtShares;
        if (debtShares != params.debtShares) {
            revert DebtChanged();
        }

Since anyone can reduce loans[params.tokenId].debtShares by calling V3Vault.sol#repay, if the liquidation is front-runn by bad actors, or it is delayed due to low gas fee, the transaction will revert.

Impact

Too strict debtShares check leads to the liquidation transaction revert.

Proof of Concept

Consider the following scenario: grieffing attack by malicious attackers

1. Alice's position is in a liquidable state, so another user prepare to liquidate her position.
2. The malicious attacker repays 1 share(cost is negligible) to decrease debtShares before liquidation.
3. The liquidation transaction revert.

The above scenario does not necessarily require an actual attacker, as long as debtShares changes before liquidation, the transaction will revert.

Tools Used

Manual Code Review

Recommended Mitigation Steps

Use <= instead of ==

        uint256 debtShares = loans[params.tokenId].debtShares;
--      if (debtShares != params.debtShares) {
++      if (debtShares <= params.debtShares) {
            revert DebtChanged();
        }

Assessed type

DoS