QA Report

[code423n4]

Note: throughout this report, findings related to NFTPair.sol also apply to NFTPairWithOracle.sol. Rather than note that everything applies to both contracts, I'll note explicitly if a finding only applies to one or the other.

Initializer can be frontrun

If a clone contract is not deployed and initialized in the same transaction, the init function can be frontrun. Since clones are meant to be deployed and initiated atomically using the BoringFactory, this is unlikely, but note that this is possible if a clone contract is deployed outside the factory.

Functions can be declared external

Several public functions are not called internally and can be declared external:

• NFTPair#init
• NFTPair#updateLoanParams
• NFTPair#withdrawFees
• NFTPair#setFeeTo

Missing zero address check in init

NFTPair#init validates that the collateral address is not address(0), but does not check the asset address.

Missing zero address check in setFeeTo

NFTPair#setFeeTo does not check that the newFeeTo parameter is not address(0).

Avoid use of native ecrecover

NFTPair#requestAndBorrow and NFTPair#takeCollateralAndLend use the native ecrecover function, which is susceptible to signature malleability. Since these functions also use a nonce, this can't be exploited for a replay attack as implemented, but note that this could be a vulnerability if used without a nonce. Consider using OpenZeppelin's ECDSA library, which rejects malleable signatures.

Effects after interactions in _requestLoan

_requestLoan changes state after calling collateral.transferFrom. It's safer and more idiomatic to perform interactions after state changing effects.

NFTPair.sol#_requestLoan

    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);
    }

Recommendation: Move the collateral transfer interaction after state changing effects. For example:

    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");
        }

        tokenLoan[tokenId].borrower = to;
        tokenLoan[tokenId].status = LOAN_REQUESTED;
        tokenLoanParams[tokenId] = params;

        if (!skim) collateral.transferFrom(collateralProvider, address(this), tokenId);

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

Incorrect comment on signature hashes

Comments describing the format of lend and borrow signature hashes mention including the asset and collateral address in the signature hash, but these hashes are not actually constructed with the asset/collateral address:

NFTPair#L337-L345

    // NOTE on signature hashes: the domain separator only guarantees that the
    // chain ID and master contract are a match, so we explicitly include the
    // clone address (and the asset/collateral addresses):

    // keccak256("Lend(address contract,uint256 tokenId,bool anyTokenId,uint128 valuation,uint64 duration,uint16 annualInterestBPS,uint256 nonce,uint256 deadline)")
    bytes32 private constant LEND_SIGNATURE_HASH = 0x06bcca6f35b7c1b98f11abbb10957d273a681069ba90358de25404f49e2430f8;

    // keccak256("Borrow(address contract,uint256 tokenId,uint128 valuation,uint64 duration,uint16 annualInterestBPS,uint256 nonce,uint256 deadline)")
    bytes32 private constant BORROW_SIGNATURE_HASH = 0xf2c9128b0fb8406af3168320897e5ff08f3bb536dd5f804c29ed276e93ec4336;

Unused library

NFTPair includes and uses the RebaseLibrary library, but the Rebase type is unused.

Duplicated interfaces

Both NFTPair.sol and NFTPairWithOracle.sol share the same ILendingClub and INFTPair interfaces defined inline at the top of the contract code. It's recommended to extract each interface to a reusable file to remove duplication, enable reuse, and avoid errors.

Unused function in ILendingClub

The ILendingClub interface defines a lendingConditions function, but it is unused throughout the codebase.

[cryptolyndon]

Nice report, thank you. Communicated clearly and efficiently, and without overly far-fetched issues to sift through.

(The zero check in init() only serves to ensure the contract does not get called twice. Passing zero in setFeeTo() does no harm other than having to update it again.)