AbraNFT contest - BowTiedWardens's results

Lines of code

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

Vulnerability details

Issue: Arbitrary oracles are permitted on construction of loans, and there is no check that the lender agrees to the used oracle.

Consequences: A borrower who requests a loan with a malicious oracle can avoid legitimate liquidation.

Proof of Concept

• Borrower requests loan with an malicious oracle
• Lender accepts loan unknowingly
• Borrowers's bad oracle is set to never return a liquidating rate on oracle.get call.
• Lender cannot call removeCollateral to liquidate the NFT when it should be allowed, as it will fail the check on L288
• To liquidate the NFT, the lender would have to whitehat along the lines of H-01, by atomically updating to an honest oracle and calling removeCollateral.

Mitigations

• Add require(params.oracle == accepted.oracle) as a condition in _lend
• Consider only allowing whitelisted oracles, to avoid injection of malicious oracles at the initial loan request stage

Lines of code

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

Vulnerability details

Issue: The oracle can be arbitrarily updated at any point in time by the lender.

Consequences: A lender can inject a malicious oracle at any time and steal the collateral NFT at the cost of his loaned tokens.

Proof of Concept

• Borrower requests loan with an honest oracle
• Malicious lender accepts loan
• Lender calls updateLoanParams and injects a malicious oracle
• Lender's bad oracle is set to return a liquidating rate on oracle.get call.
• Lender calls removeCollateral and liquidates the NFT

Mitigations

Do not allow the oracle to be changed after the loan is set. This could potentially be accomplished by moving the oracle as a permanent parameter in the TokenLoan struct vs. a transient one in the TokenLoanParams struct.

Lines of code

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

Vulnerability details

Issue: updateLoanParams allows the lender to change the terms of an in-progress loan to lower ltvBPS. removeCollateral calculates whether liquidation is allowed via require(rate.mul(loanParams.ltvBPS) / BPS < amount, "NFT is still valued");. A low or 0 ltvBPS effectively bypasses the oracle price.

Consequences: A malicious lender can steal an NFT at the cost of his lent tokens.

Proof of Concept

• Borrower requests loan
• Lender accepts loan
• Lender calls updateLoanParams with 0 as ltvBPS
• Lender calls removeCollateral, liquidating the NFT no matter the actual oracle price

Mitigations

• Only allow lender to raise ltvBPS, not lower it
• Consider not allowing ltvBPS to be modified after the loan is made
• Add input validation to ltvBPS.
• Adjust direction of check in _lend to correctly match the fact that lower ltvBPS is better for the lender.

• [L-01] Immutable addresses should be 0-checked
• [L-02] Prevent accidentally burning tokens
• [L-03] Use of ecrecover is susceptible to signature malleability

[L-01] Immutable addresses should be 0-checked

Consider adding an address(0) check here:

186:     constructor(IBentoBoxV1 bentoBox_) public {
187:         bentoBox = bentoBox_; //@audit missing address(0) check 
188:         masterContract = this;
189:     }

[L-02] Prevent accidentally burning tokens

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

Consider adding a check to prevent accidentally burning tokens here:

668:             } else if (action == ACTION_BENTO_TRANSFER) {
669:                 (IERC20 token, address to, int256 share) = abi.decode(datas[i], (IERC20, address, int256));
670:                 bentoBox.transfer(token, msg.sender, to, _num(share, value1, value2));

[L-03] Use of ecrecover is susceptible to signature malleability

The ecrecover function is used to verify and execute Meta transactions. The built-in EVM precompile ecrecover is susceptible to signature malleability (because of non-unique s and v values) which could lead to replay attacks.

Consider using OpenZeppelin’s ECDSA library

Table of Contents:

• NFTPair.init and NFTPairWithOracle.init: Storage usage optimization
• Caching storage values in memory
• Splitting require() statements that use && saves gas
• An array's length should be cached to save gas in for-loops
• ++i costs less gas compared to i++ or i += 1
• Public functions to external
• updateLoanParams(): Replace memory with calldata and public with external
• updateLoanParams(): Copying in memory can be more expensive than using the storage keyword
• _lend(): Copying in memory can be more expensive than using the storage keyword
• No need to explicitly initialize variables with default values
• Reduce the size of error messages (Long revert Strings)

NFTPair.init and NFTPairWithOracle.init: Storage usage optimization

I suggest replacing:

175:     function init(bytes calldata data) public payable override {
176:         require(address(collateral) == address(0), "NFTPair: already initialized");
177:         (collateral, asset) = abi.decode(data, (IERC721, IERC20));
178:         require(address(collateral) != address(0), "NFTPair: bad pair"); //@audit could save 1 SLOAD here + refund 2 SSTOREs on revert
179:     }

with:

function init(bytes calldata data) public payable override {
    require(address(collateral) == address(0), "NFTPair: already initialized");
    (address _collateral, address _asset) = abi.decode(data, (IERC721, IERC20));
    require(address(_collateral) != address(0), "NFTPair: bad pair");
    (collateral, asset) = (_collateral, _asset);
}

Here, we're saving 1 SLOAD at the cost of 2 MSTOREs and 3 MLOADs => around 85 gas. Furthermore, in case of revert, a lot more gas would be refunded, as the 2 SSTORE operations are done after the require statements.

Caching storage values in memory

The code can be optimized by minimising the number of SLOADs. SLOADs are expensive (100 gas) compared to MLOADs/MSTOREs (3 gas). Here, storage values should get cached in memory (see the @audit tags for further details):

contracts/NFTPair.sol:
  290:         uint256 totalShare = bentoBox.toShare(asset, params.valuation, false); //@audit gas: asset SLOAD 1
  297:                 bentoBox.balanceOf(asset, address(this)) >= (totalShare - openFeeShare + protocolFeeShare + feesEarnedShare),  //@audit gas: asset SLOAD 2
  301:             bentoBox.transfer(asset, lender, address(this), totalShare - openFeeShare + protocolFeeShare);  //@audit gas: asset SLOAD 2
  305:         bentoBox.transfer(asset, address(this), loan.borrower, borrowerShare);  //@audit gas: asset SLOAD 3
  523:         uint256 totalShare = bentoBox.toShare(asset, amount, false); //@audit gas: asset SLOAD 1
  524:         uint256 feeShare = bentoBox.toShare(asset, fee, false); //@audit gas: asset SLOAD 2
  528:             require(bentoBox.balanceOf(asset, address(this)) >= (totalShare + feesEarnedShare), "NFTPair: skim too much");  //@audit gas: asset SLOAD 3
  532:             bentoBox.transfer(asset, msg.sender, address(this), feeShare); //@audit gas: asset SLOAD 3
  539:         bentoBox.transfer(asset, from, loan.lender, totalShare - feeShare); //@audit gas: asset SLOAD 4

contracts/NFTPairWithOracle.sol:
  325:         uint256 totalShare = bentoBox.toShare(asset, params.valuation, false); //@audit gas: asset SLOAD 1
  332:                 bentoBox.balanceOf(asset, address(this)) >= (totalShare - openFeeShare + protocolFeeShare + feesEarnedShare), //@audit gas: asset SLOAD 2
  336:             bentoBox.transfer(asset, lender, address(this), totalShare - openFeeShare + protocolFeeShare); //@audit gas: asset SLOAD 2
  340:         bentoBox.transfer(asset, address(this), loan.borrower, borrowerShare); //@audit gas: asset SLOAD 3
  556:         uint256 totalShare = bentoBox.toShare(asset, amount, false); //@audit gas: asset SLOAD 1
  557:         uint256 feeShare = bentoBox.toShare(asset, fee, false); //@audit gas: asset SLOAD 2
  561:             require(bentoBox.balanceOf(asset, address(this)) >= (totalShare + feesEarnedShare), "NFTPair: skim too much"); //@audit gas: asset SLOAD 3
  565:             bentoBox.transfer(asset, msg.sender, address(this), feeShare); //@audit gas: asset SLOAD 3
  572:         bentoBox.transfer(asset, from, loan.lender, totalShare - feeShare); //@audit gas: asset SLOAD 4

Splitting require() statements that use && saves gas

Instead of using the && operator in a single require statement to check multiple conditions, I suggest using multiple require statements with 1 condition per require statement (saving 3 gas per &):

contracts/NFTPair.sol:
  622:         require(callee != address(bentoBox) && callee != address(collateral) && callee != address(this), "NFTPair: can't call");

contracts/NFTPairWithOracle.sol:
  655:         require(callee != address(bentoBox) && callee != address(collateral) && callee != address(this), "NFTPair: can't call");

An array's length should be cached to save gas in for-loops

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Here, I suggest storing the array's length in a variable before the for-loop, and use it instead:

NFTPair.sol:641:        for (uint256 i = 0; i < actions.length; i++) {
NFTPairWithOracle.sol:674:        for (uint256 i = 0; i < actions.length; i++) {

++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;  
i++; // == 1 but i == 2  

But ++i returns the actual incremented value:

uint i = 1;  
++i; // == 2 and i == 2 too, so no need for a temporary variable  

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2

Instances include:

NFTPair.sol:494:        for (uint256 k = 2; k <= COMPOUND_INTEREST_TERMS; k++) {
NFTPair.sol:641:        for (uint256 i = 0; i < actions.length; i++) {
NFTPairWithOracle.sol:527:        for (uint256 k = 2; k <= COMPOUND_INTEREST_TERMS; k++) {
NFTPairWithOracle.sol:674:        for (uint256 i = 0; i < actions.length; i++) {

I suggest using ++i instead of i++ to increment the value of an uint variable.

Public functions to external

The following functions could be set external to save gas and improve code quality.

    NFTPair.init(bytes) (contracts/NFTPair.sol#175-179)
    NFTPairWithOracle.init(bytes) (contracts/NFTPairWithOracle.sol#192-196)
    NFTPair.updateLoanParams(uint256,TokenLoanParams) (contracts/NFTPair.sol#181-203)
    NFTPair.withdrawFees() (contracts/NFTPair.sol#713-723)
    NFTPair.setFeeTo(address) (contracts/NFTPair.sol#728-731)
    NFTPairWithOracle.updateLoanParams(uint256,TokenLoanParams) (contracts/NFTPairWithOracle.sol#198-223)
    NFTPairWithOracle.withdrawFees() (contracts/NFTPairWithOracle.sol#735-745)
    NFTPairWithOracle.setFeeTo(address) (contracts/NFTPairWithOracle.sol#750-753)

updateLoanParams(): Replace memory with calldata and public with external

This is valid in both files NFTPair.sol and NFTPairWithOracle.sol. As mentioned above, updateLoanParams() should be external. Furthermore, TokenLoanParams memory params should be TokenLoanParams calldata params. Therefore, we'd go from:

function updateLoanParams(uint256 tokenId, TokenLoanParams memory params) public

to

function updateLoanParams(uint256 tokenId, TokenLoanParams calldata params) external

updateLoanParams(): Copying in memory can be more expensive than using the storage keyword

This is valid in both files NFTPair.sol and NFTPairWithOracle.sol. In this particular case here, I suggest using the storage keyword instead of the memory one, as the copy in memory is wasting some MSTOREs and MLOADs. See the @audit tags for more details:

    function updateLoanParams(uint256 tokenId, TokenLoanParams memory params) public {
        TokenLoan memory loan = tokenLoan[tokenId]; //@audit gas: use the storage keyword instead and only cache loan.status
        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];  //@audit gas: copying in memory is actually more expensive in this use-case than using storage
            require(
                params.duration >= cur.duration &&
                    params.valuation <= cur.valuation &&
                    params.annualInterestBPS <= cur.annualInterestBPS &&
                    params.ltvBPS <= cur.ltvBPS,
                "NFTPair: worse params"
            );
        } 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 {
          (...)

I suggest:

• Using TokenLoan storage loan = tokenLoan[tokenId];
• Only caching loan.status in memory as it can be evaluated twice (in the if/else statement)
• Using TokenLoanParams storage cur = tokenLoanParams[tokenId];

_lend(): Copying in memory can be more expensive than using the storage keyword

In this function, I suggest replacing TokenLoan memory loan = tokenLoan[tokenId]; with TokenLoan storage loan = tokenLoan[tokenId];. Only 2 SLOADs are made (loan.status and loan.borrower) and the function is writing in memory (loan variable) before writing in storage. These steps are superfluous and there's no value from a copy in memory here.

No need to explicitly initialize variables with default values

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {

Instances include:

NFTPair.sol:96:    uint8 private constant LOAN_INITIAL = 0;
NFTPair.sol:641:        for (uint256 i = 0; i < actions.length; i++) {
NFTPairWithOracle.sol:113:    uint8 private constant LOAN_INITIAL = 0;
NFTPairWithOracle.sol:674:        for (uint256 i = 0; i < actions.length; i++) {

I suggest removing explicit initializations for default values.

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

NFTPair.sol:366:            require(ILendingClub(lender).willLend(tokenId, params), "NFTPair: LendingClub does not like you"); //@audit length == 38
NFTPairWithOracle.sol:398:            require(ILendingClub(lender).willLend(tokenId, params), "NFTPair: LendingClub does not like you"); //@audit length == 38

I suggest shortening the revert strings to fit in 32 bytes, or using custom errors as described next.