AbraNFT contest - plotchy's results

Lines of code

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

Vulnerability details

Impact

Any temporary failure in an oracle relaying a price allows the NFT collateral to be removed by the lender, even if the value of the NFT is still far above the agreed-upon liquidation value.

Considering that oracle price retrieval failure is accounted for in the design of INFTOracle, this should be also be accounted for within NFTPairWithOracle.

Proof of Concept

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/interfaces/INFTOracle.sol#L5-L10 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L287 NFTPairWithOracle allows an Oracle to relay the price of the collateral. The get() function of INFTOracle is used for this and is defined as the following:

/// @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);

The bool success return variable is defined to be true on a successful price retrieval and false otherwise. The uint256 rate return variable will have a retrieved uint256 value if the call is successful and a default value of 0 otherwise.

In NFTPairWithOracle.sol, this get() function is used within the removeCollateral() function to see if the current value is higher than the liquidation price.

/// @notice Removes `tokenId` as collateral and transfers it to `to`.
    /// @notice This destroys the loan.
    /// @param tokenId The token
    /// @param to The receiver of the token.
    function removeCollateral(uint256 tokenId, address to) public {
        TokenLoan memory loan = tokenLoan[tokenId];
        if (loan.status == LOAN_REQUESTED) {
            ...
        } 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); //@audit here
                require(rate.mul(loanParams.ltvBPS) / BPS < amount, "NFT is still valued");
            }
        }
        
        delete tokenLoan[tokenId];
        collateral.transferFrom(address(this), to, tokenId);
        emit LogRemoveCollateral(tokenId, to);
    }

If the get() function finds that there is no valid (recent) rate available, it will return (0,0) and have rate = 0. This will make the following require() statement always pass and allow the collateral to be removed.

Recommended Mitigation Steps

Check that the INFTOracle get() function has a successful oracle result. Do this by checking bool success before continuing with the liquidation check.

(bool success, uint256 rate) = loanParams.oracle.get(address(this), tokenId);
require(success, "Oracle call failed");
require(rate.mul(loanParams.ltvBPS) / BPS < amount, "NFT is still valued");

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L316

Vulnerability details

Impact

While loan is in LOAN_REQUESTED phase, the updateLoanParameters() function can be called by the borrower to adjust TokenLoanParams.ltvBPS to a value that is very unfavorable for a prospective lender.

Usually, this results in the loan not being accepted by a counterparty, but the loan accepting transaction can be sniped en-route by the borrower upon seeing a lender call lend() in the mempool. The TokenLoanParams.ltvBPS can be updated to type(uint16).max and create a near-unliquidatable loan.

The following example shows how impactful this can be: collateral = BAYC ($350k usd) borrower is seeking a low-value loan with low interest and long duration

initial TokenLoanParams: valuation = $50k duration = 12 months annualInterestBPS = 0% ltvBPS = 1/5 equivalent (liqudating price = BAYC at $250k)

A prospective lender can see this and take on the loan due to the high-return should the BAYC become liquidatable. (50k -> 250k return should the BAYC drop by ~30% in value)

However, this .ltvBPS value can be updated and increased to the maximal value en-route (becoming 6.5x ltv equivalent), and create a loan only liquidatable if the BAYC reaches 50k/6.5 = ~$8k, effectively stealing the lender's capital for the entirety of the loan duration.

Proof of Concept

The issue resides in _lend() having the opposite sign in the require statement for checking the ltvBPS value with what was accepted by the lender. https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L316

require(
        params.valuation == accepted.valuation &&
        params.duration <= accepted.duration &&
        params.annualInterestBPS >= accepted.annualInterestBPS &&
        params.ltvBPS >= accepted.ltvBPS, //@audit this sign should be flipped
        "NFTPair: bad params"
        );

updateLoanParams allows the borrower to update the parameters at any time before the loan becomes LOAN_OUTSTANDING

function updateLoanParams(uint256 tokenId, TokenLoanParams memory params) public {
        TokenLoan memory loan = tokenLoan[tokenId];
        ...
        } else if (loan.status == LOAN_REQUESTED) {
            // The borrower has already deposited the collateral and can
            // change whatever they like
            require(msg.sender == loan.borrower, "NFTPair: not the borrower");
        ...
        tokenLoanParams[tokenId] = params; //@audit loan parameters are updated.
        emit LogUpdateLoanParams(tokenId, params.valuation, params.duration, params.annualInterestBPS, params.ltvBPS);
    }

Setting it to a high amount (such as type(uint16).max) basically prevents the liquidation check from ever passing in removeCollateral() https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L288

function removeCollateral(uint256 tokenId, address to) public {
        TokenLoan memory loan = tokenLoan[tokenId];
        if (loan.status == LOAN_REQUESTED) {
            ...
        } 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"); //@audit ltvBPS as uint16.max makes this check nearly-never pass
            }
        }
        ...
        delete tokenLoan[tokenId];
        collateral.transferFrom(address(this), to, tokenId);
        emit LogRemoveCollateral(tokenId, to);
    }

Recommended Mitigation Steps

Change _lend() to have params.ltvBPS >= accepted.ltvBPS changed to params.ltvBPS <= accepted.ltvBPS as this is better for the lender.