AbraNFT contest - WatchPug's results

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L287

Vulnerability details

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/interfaces/INFTOracle.sol#L8

interface INFTOracle {
    /// @notice Get the latest exchange rate.
    /// @param pair address of the NFTPair calling the oracle
    /// @param tokenId tokenId of the NFT in question 
    /// @return success if no valid (recent) rate is available, return false else true.
    /// @return rate The rate of the requested asset / pair / pool.
    function get(address pair, uint256 tokenId) external returns (bool success, uint256 rate);

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L287

(, uint256 rate) = loanParams.oracle.get(address(this), tokenId);

Per the comment of the INFTOracle, the first return (success) of INFTOracle.get() means:

success: if no valid (recent) rate is available, return false else true.

However, in removeCollateral, the success return of loanParams.oracle.get() is ignored. Making it possible for the loan to be liquidated unfairly with an invalid or stale price.

Specifically, if the oracle returns a 0 for rate or a stale, much lower rate than the actual value, then the collateral NFT can be impounded by the lender immediately. Which will cause fund loss to the borrower.

Recommendation

Consider adding a check for success:

(bool success, uint256 rate) = loanParams.oracle.get(address(this), tokenId);
require(success);

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L205-L211

Vulnerability details

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L205-L211

require(
    params.duration >= cur.duration &&
        params.valuation <= cur.valuation &&
        params.annualInterestBPS <= cur.annualInterestBPS &&
        params.ltvBPS <= cur.ltvBPS,
    "NFTPair: worse params"
);

As per the comment:

The lender can change terms so long as the changes are strictly the same or better for the borrower.

However, the implementation is wrong for ltvBPS. In the current implementation, it requires the new ltvBPS to be lower than the existing ltvBPS. Which is actually a worse term for the borrower.

A malicious lender can take advantage of this, and set the new ltvBPS to a lower value or even 0, and then call removeCollateral() to impound the NFT collateral from the borrower.

Recommendation

Change to:

require(
    params.duration >= cur.duration &&
        params.valuation <= cur.valuation &&
        params.annualInterestBPS <= cur.annualInterestBPS &&
        params.ltvBPS >= cur.ltvBPS,
    "NFTPair: worse params"
);

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L312-L318

Vulnerability details

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L312-L318

 require(
    params.valuation == accepted.valuation &&
        params.duration <= accepted.duration &&
        params.annualInterestBPS >= accepted.annualInterestBPS &&
        params.ltvBPS >= accepted.ltvBPS,
    "NFTPair: bad params"
);

As per the comment:

Valuation has to be an exact match, everything else must be at least as good for the lender as accepted.

However, the implementation is wrong for ltvBPS. In the current implementation, it requires the new ltvBPS to be lower than the existing ltvBPS. Which is actually a worse term for the borrower.

A malicious borrower can take advantage of this, and frontrun the lender's lend() tx to set ltvBPS to a higher value or even as high as near type(uint256).max, to prevent the loan from being liquidated even when the price of the collateral drops to below the principal.

PoC

1. The borrower requestLoan() for $30k with a NFT worth $1M with a ltvBPS of 5000 (50%);
2. The lender lend() $30k with accepted terms of ltvBPS of 5000;
3. The borrower fronrun lend() with updateLoanParams() and set ltvBPS to 1000000 (10000%).
4. The value of the NFT collateral drops to $50k and then $20k, and the lender cant removeCollateral() as "NFT is still valued".

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L288

require(rate.mul(loanParams.ltvBPS) / BPS < amount, "NFT is still valued");

Recommendation

Change to:

 require(
    params.valuation == accepted.valuation &&
        params.duration <= accepted.duration &&
        params.annualInterestBPS >= accepted.annualInterestBPS &&
        params.ltvBPS <= accepted.ltvBPS,
    "NFTPair: bad params"
);

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L267-L297

Vulnerability details

NFT is a fragmented standard, for certain non-standard ERC721 implementations, they may have built-in hooks that can be used to re-enter the contract. Just like ERC777 to ERC20.

For example, if the collateral NFT got a pre-transfer hook to the receiver of the transfer, then it can be used to re-enter the contract and requestLoan without depositing the collateral.

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L267-L297

    function removeCollateral(uint256 tokenId, address to) public {
        TokenLoan memory loan = tokenLoan[tokenId];
        if (loan.status == LOAN_REQUESTED) {
            // We are withdrawing collateral that is not in use:
            require(msg.sender == loan.borrower, "NFTPair: not the borrower");
        } else if (loan.status == LOAN_OUTSTANDING) {
            // We are seizing collateral towards the lender. The loan has to be
            // expired and not paid off, or underwater and not paid off:
            require(to == loan.lender, "NFTPair: not the lender");

            if (uint256(loan.startTime) + tokenLoanParams[tokenId].duration > block.timestamp) {
                TokenLoanParams memory loanParams = tokenLoanParams[tokenId];
                // No underflow: loan.startTime is only ever set to a block timestamp
                // Cast is safe: if this overflows, then all loans have expired anyway
                uint256 interest = calculateInterest(
                    loanParams.valuation,
                    uint64(block.timestamp - loan.startTime),
                    loanParams.annualInterestBPS
                ).to128();
                uint256 amount = loanParams.valuation + interest;
                (, uint256 rate) = loanParams.oracle.get(address(this), tokenId);
                require(rate.mul(loanParams.ltvBPS) / BPS < amount, "NFT is still valued");
            }
        }
        // If there somehow is collateral but no accompanying loan, then anyone
        // can claim it by first requesting a loan with `skim` set to true, and
        // then withdrawing. So we might as well allow it here..
        delete tokenLoan[tokenId];
        collateral.transferFrom(address(this), to, tokenId);
        emit LogRemoveCollateral(tokenId, to);
    }

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPairWithOracle.sol#L225-L247

    function _requestLoan(
        address collateralProvider,
        uint256 tokenId,
        TokenLoanParams memory params,
        address to,
        bool skim
    ) private {
        // Edge case: valuation can be zero. That effectively gifts the NFT and
        // is therefore a bad idea, but does not break the contract.
        require(tokenLoan[tokenId].status == LOAN_INITIAL, "NFTPair: loan exists");
        if (skim) {
            require(collateral.ownerOf(tokenId) == address(this), "NFTPair: skim failed");
        } else {
            collateral.transferFrom(collateralProvider, address(this), tokenId);
        }
        TokenLoan memory loan;
        loan.borrower = to;
        loan.status = LOAN_REQUESTED;
        tokenLoan[tokenId] = loan;
        tokenLoanParams[tokenId] = params;

        emit LogRequestLoan(to, tokenId, params.valuation, params.duration, params.annualInterestBPS, params.ltvBPS);
    }

PoC

The malicious borrower can:

1. requestLoan() with a collateral tokenId: 123;
2. removeCollateral() for tokenId: 123:
   • in the pre-transfer hook of collateral.transferFrom(address(this), to, tokenId);, re-enter requestLoan() with tokenId: 123 and skim: true
3. continue with transfer and the NFT is now back to the borrower.

As a result, the malicious borrower has effectively created a loan without depositing the collateral NFT.

Recommendation

Consider adding require(collateral.ownerOf(tokenId) == address(this)); in _lend() to make sure the collateral tokenId is owned by the contract.

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L540

Vulnerability details

https://github.com/code-423n4/2022-04-abranft/blob/5cd4edc3298c05748e952f8a8c93e42f930a78c2/contracts/NFTPair.sol#L540

    collateral.transferFrom(address(this), loan.borrower, tokenId);

If loan.borrower is a contract that does not implement the onERC721Received method, in the current implementation of repay(), the tx will be successful, and the NFT will be transferred.

This can be a problem if loan.borrower can not handle ERC721 properly.

As per the documentation of EIP-721:

A wallet/broker/auction application MUST implement the wallet interface if it will accept safe transfers.

Ref: https://eips.ethereum.org/EIPS/eip-721

Recommendation

Consider using safeTransferFrom in NFTPair.sol#repay()):

    collateral.safeTransferFrom(address(this), loan.borrower, tokenId);