code-423n4 / 2024-04-revert-mitigation-findings

1 stars 1 forks source link

H-06 MitigationConfirmed #36

Open c4-bot-6 opened 4 months ago

c4-bot-6 commented 4 months ago

Lines of code

Vulnerability details

C4 issue

H-06: Owner of a position can prevent liquidation due to the 'onERC721Received' callback

Comment

The original code will send the NFT back to the owner when their loan is liquidated:

 function liquidate(LiquidateParams calldata params) external override returns (uint256 amount0, uint256 amount1) {
...
_cleanupLoan(params.tokenId, state.newDebtExchangeRateX96, state.newLendExchangeRateX96, 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);
    }

This allows a borrower to avoid liquidation by using a smart contract and revert on onERC721Received function.

Mitigation

PR #8, PR #32

The mitigation code now no longer transfer nft back in liquidate function:

function _cleanupLoan(uint256 tokenId, uint256 debtExchangeRateX96, uint256 lendExchangeRateX96) internal {
        _updateAndCheckCollateral(tokenId, debtExchangeRateX96, lendExchangeRateX96, loans[tokenId].debtShares, 0);
        delete loans[tokenId];
    }

In stead, it only deletes the loan, the nft owner can then call remove to get their nft back:

function remove(uint256 tokenId, address recipient, bytes calldata data) external {
        address owner = tokenOwner[tokenId];
        if (owner != msg.sender) {
            revert Unauthorized();
        }

        if (loans[tokenId].debtShares != 0) {
            revert NeedsRepay();
        }

        _removeTokenFromOwner(owner, tokenId);
        nonfungiblePositionManager.safeTransferFrom(address(this), recipient, tokenId, data);
        emit Remove(tokenId, owner, recipient);
    }

The mitigation solved the original issue.

Conclusion

LGTM

c4-judge commented 4 months ago

jhsagd76 marked the issue as satisfactory