AbraNFT contest - joestakey's results

QA Report

Table of Contents

• summary
• Comment Missing function parameter
• Event should use three indexed fields if possible
• Function missing comments
• Related data should be grouped in struct
• Revert should have an error message
• Unchecked inputs
• Visibility for constructor not needed
• Array lengths should be checked if expected to match

summary

Only non-critical vulnerabilities were found, the most notable one being the lack of array length check before computation in some for loops

Comment Missing function parameter

PROBLEM

Some of the function comments are missing function parameters or returns

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:169 IBentoBoxV1 bentoBox_
NFTPair.sol:175 bytes calldata data
NFTPair.sol:353 uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s
NFTPair.sol:395 uint256 deadline,
        uint8 v,
        bytes32 r,
        bytes32 s

NFTPairWithOracle.sol

NFTPairWithOracle.sol:186 IBentoBoxV1 bentoBox_
NFTPairWithOracle.sol:192 bytes calldata data
NFTPairWithOracle.sol:388 SignatureParams memory signature
NFTPairWithOracle.sol:429 SignatureParams memory signature

TOOLS USED

Manual Analysis

MITIGATION

Add a comment for these parameters

Event should use three indexed fields if possible

PROBLEM

Each event should use three indexed fields if there are three or more fields

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:65 event LogRequestLoan(address indexed borrower, uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS);
NFTPair.sol:66 event LogUpdateLoanParams(uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS);

NFTPairWithOracle.sol

NFTPairWithOracle.sol:75 event LogRequestLoan(address indexed borrower, uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS);
NFTPairWithOracle.sol:83 event LogUpdateLoanParams(uint256 indexed tokenId, uint128 valuation, uint64 duration, uint16 annualInterestBPS);

TOOLS USED

Manual Analysis

MITIGATION

Add indexed to some fields of these events to reach a number of three indexed fields.

Function missing comments

PROBLEM

Some functions are missing comments.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:181: function updateLoanParams(uint256 tokenId, TokenLoanParams memory params)
NFTPair.sol:505: function repay(uint256 tokenId, bool skim)

NFTPairWithOracle.sol

NFTPairWithOracle.sol:198: function updateLoanParams(uint256 tokenId, TokenLoanParams memory params)
NFTPairWithOracle.sol:538: function repay(uint256 tokenId, bool skim)

TOOLS USED

Manual Analysis

MITIGATION

Add comments to these functions

Related data should be grouped in struct

PROBLEM

When there are mappings that use the same key value, having separate fields is error prone, for instance in case of deletion or with future new fields.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:91: mapping(uint256 => TokenLoanParams) public tokenLoanParams;
NFTPair.sol:105: mapping(uint256 => TokenLoan) public tokenLoan;

NFTPairWithOracle.sol

NFTPairWithOracle.sol:111: mapping(uint256 => TokenLoanParams) public tokenLoanParams;
NFTPairWithOracle.sol:122: mapping(uint256 => TokenLoan) public tokenLoan;

TOOLS USED

Manual Analysis

MITIGATION

Group the related data in a struct and use one mapping. For instance, for the NFTPair.sol mappings, the mitigation could be:

struct TokenLoans {
  TokenLoanParams tokenLoanParams;
  TokenLoan tokenLoan;
}

And it would be used as a state variable:

mapping(uint256 =>  TokenLoans) tokenLoans;

Revert should have an error message

PROBLEM

revert() statements should always have an error message so that revert errors can be detected more easily.

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:501: revert();

NFTPairWithOracle.sol

NFTPairWithOracle.sol:534: revert();

TOOLS USED

Manual Analysis

MITIGATION

Add an error message to the revert statements.

Unchecked inputs

PROBLEM

There should be a non-zero (address or integer) check in all setters

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:728: setFeeTo()

NFTPairWithOracle.sol

NFTPairWithOracle.sol:750: setFeeTo()

TOOLS USED

Manual Analysis

MITIGATION

Add a non-zero check:

require(newFeeTo != address(0));

Visibility for constructor not needed

PROBLEM

Visibility (public / external) is not needed for constructors anymore since Solidity 0.7.0, see here

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:169

NFTPairWithOracle.sol

NFTPairWithOracle.sol:186

TOOLS USED

Manual Analysis

MITIGATION

Remove public modifier from constructors.

Array lengths should be checked if expected to match

PROBLEM

Array lengths should be checked if expected to match in a for loop, to provide users with a clear error message if their arrays inputs do not have suitable lengths

SEVERITY

Non-Critical

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:636: function cook(
        uint8[] calldata actions,
        uint256[] calldata values,
        bytes[] calldata datas
    )

NFTPairWithOracle.sol

NFTPairWithOracle.sol:669: function cook(
        uint8[] calldata actions,
        uint256[] calldata values,
        bytes[] calldata datas
    )

TOOLS USED

Manual Analysis

MITIGATION

Add a lengths check:

if ((actions.length != values.length) || (actions.length != datas.length)) {
        revert ErrorArrayLength(actions, values, datas);
}

Gas Report

Table of Contents

• Caching storage variables in memory to save gas
• Calldata instead of memory for RO function parameters
• Comparison operators
• Constructor parameters should be avoided when possible
• Custom errors
• Dead code
• Default value initialization
• Prefix increments
• Require instead of AND
• Tight variable packing
• unchecked arithmetic

Caching storage variables in memory to save gas

IMPACT

Anytime you are reading from storage more than once, it is cheaper in gas cost to cache the variable in memory: a SLOAD cost 100gas, while MLOAD and MSTORE cost 3 gas.

In particular, in for loops, when using the length of a storage array as the condition being checked after each loop, caching the array length in memory can yield significant gas savings if the array length is high

PROOF OF CONCEPT

Instances include:

NFTPair.sol

scope: _lend()

• asset is read 3 times (line 290, then either line 297 or 301 based on conditions, then line 305)

NFTPair.sol:290
NFTPair.sol:297
NFTPair.sol:301
NFTPair.sol:305

scope: repay()

• asset is read 4 times (line 523, 524, then either line 528 or 532 based on conditions, then line 539)

NFTPair.sol:523
NFTPair.sol:524
NFTPair.sol:528
NFTPair.sol:532
NFTPair.sol:539

NFTPairWithOracle.sol

scope: _lend()

• asset is read twice (line 325, then either line 332 or 336 based on conditions, then line 340)

NFTPairWithOracle.sol:325
NFTPairWithOracle.sol:332
NFTPairWithOracle.sol:336
NFTPairWithOracle.sol:340

scope: repay()

• asset is read 4 times (line 556, 557, then either line 561 or 565 based on conditions, then line 572)

NFTPairWithOracle.sol:556
NFTPairWithOracle.sol:557
NFTPairWithOracle.sol:561
NFTPairWithOracle.sol:565
NFTPairWithOracle.sol:572

TOOLS USED

Manual Analysis

MITIGATION

cache these storage variables in memory

Calldata instead of memory for RO function parameters

PROBLEM

If a reference type function parameter is read-only, it is cheaper in gas to use calldata instead of memory. Calldata is a non-modifiable, non-persistent area where function arguments are stored, and behaves mostly like memory.

Try to use calldata as a data location because it will avoid copies and also makes sure that the data cannot be modified.

PROOF OF CONCEPT

Instances include:

NFTPair.sol

scope: _bentoDeposit()

NFTPair.sol:579: bytes memory data

scope: _bentoWithdraw()

NFTPair.sol:592: bytes memory data

scope: _call()

NFTPair.sol:605: bytes memory data

NFTPairWithOracle.sol

scope: _bentoDeposit()

NFTPairWithOracle.sol:612: bytes memory data

scope: _bentoWithdraw()

NFTPairWithOracle.sol:625: bytes memory data

scope: _call()

NFTPairWithOracle.sol:638: bytes memory data

TOOLS USED

Manual Analysis

MITIGATION

Replace memory with calldata

Comparison Operators

IMPACT

In the EVM, there is no opcode for >= or <=. When using greater than or equal, two operations are performed: > and =.

Using strict comparison operators hence saves gas

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:189
NFTPair.sol:189
NFTPair.sol:189
NFTPair.sol:258
NFTPair.sol:285
NFTPair.sol:286
NFTPair.sol:297
NFTPair.sol:368
NFTPair.sol:405
NFTPair.sol:494
NFTPair.sol:500
NFTPair.sol:528
NFTPair.sol:574

NFTPairWithOracle.sol

NFTPairWithOracle.sol:206
NFTPairWithOracle.sol:207
NFTPairWithOracle.sol:208
NFTPairWithOracle.sol:209
NFTPairWithOracle.sol:314
NFTPairWithOracle.sol:315
NFTPairWithOracle.sol:316
NFTPairWithOracle.sol:322
NFTPairWithOracle.sol:332
NFTPairWithOracle.sol:400
NFTPairWithOracle.sol:436
NFTPairWithOracle.sol:527
NFTPairWithOracle.sol:533
NFTPairWithOracle.sol:561
NFTPairWithOracle.sol:607

TOOLS USED

Manual Analysis

MITIGATION

Replace <= with <, and >= with >. Do not forget to increment/decrement the compared variable

example:

-interest >= 2**128;
+interest > 2**128 - 1;

In cases where 1 is negligible compared to the value of the variable, we can omit the increment.

Constructor parameters should be avoided when possible

IMPACT

Constructor parameters are expensive. The contract deployment will be cheaper in gas if they are hard coded instead of using constructor parameters.

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:169 IBentoBoxV1 bentoBox_

NFTPairWithOracle.sol

NFTPairWithOracle.sol:186 IBentoBoxV1 bentoBox_

TOOLS USED

Manual Analysis

MITIGATION

Hardcode bentoBox with its value instead of writing it during contract deployment with constructor parameters.

Custom Errors

IMPACT

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) while providing the same amount of information, as explained here

Custom errors are defined using the error statement

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:176
NFTPair.sol:178
NFTPair.sol:186
NFTPair.sol:188
NFTPair.sol:195
NFTPair.sol:214
NFTPair.sol:216
NFTPair.sol:251
NFTPair.sol:255
NFTPair.sol:256
NFTPair.sol:278
NFTPair.sol:283
NFTPair.sol:296
NFTPair.sol:366
NFTPair.sol:368
NFTPair.sol:383
NFTPair.sol:405
NFTPair.sol:419
NFTPair.sol:507
NFTPair.sol:509
NFTPair.sol:528
NFTPair.sol:622
NFTPair.sol:625

NFTPairWithOracle.sol

NFTPairWithOracle.sol:193
NFTPairWithOracle.sol:195
NFTPairWithOracle.sol:203
NFTPairWithOracle.sol:205
NFTPairWithOracle.sol:215
NFTPairWithOracle.sol:234
NFTPairWithOracle.sol:236
NFTPairWithOracle.sol:271
NFTPairWithOracle.sol:275
NFTPairWithOracle.sol:288
NFTPairWithOracle.sol:307
NFTPairWithOracle.sol:312
NFTPairWithOracle.sol:322
NFTPairWithOracle.sol:331
NFTPairWithOracle.sol:398
NFTPairWithOracle.sol:400
NFTPairWithOracle.sol:417
NFTPairWithOracle.sol:436
NFTPairWithOracle.sol:452
NFTPairWithOracle.sol:540
NFTPairWithOracle.sol:542
NFTPairWithOracle.sol:561
NFTPairWithOracle.sol:655
NFTPairWithOracle.sol:658

TOOLS USED

Manual Analysis

MITIGATION

Replace require and revert statements with custom errors.

For instance, in NFTPair.sol:

Replace

require(address(collateral) == address(0), "NFTPair: already initialized");

with

if (address(collateral) != address(0)) {
		revert IsAlreadyInitialized(collateral);
}

and define the custom error in the contract

error IsAlreadyInitialized(IERC721 collateral);

Dead code

IMPACT

Variables that are not used should be removed as they increase the gas cost during deployment

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:566: int256 internal constant USE_VALUE2 = -2;

NFTPairWithOracle.sol

NFTPairWithOracle.sol:599: int256 internal constant USE_VALUE2 = -2;

TOOLS USED

Manual Analysis

MITIGATION

Remove these unused variables.

Default value initialization

IMPACT

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:96: uint8 private constant LOAN_INITIAL = 0;
NFTPair.sol:641: uint256 i = 0

NFTPairWithOracle.sol

NFTPairWithOracle.sol:113: uint8 private constant LOAN_INITIAL = 0;
NFTPairWithOracle.sol:674: uint256 i = 0

TOOLS USED

Manual Analysis

MITIGATION

Remove explicit initialization for default values.

Prefix increments

IMPACT

Prefix increments are cheaper than postfix increments.

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:369
NFTPair.sol:406
NFTPair.sol:494
NFTPair.sol:641

NFTPairWithOracle.sol

NFTPair.sol:401
NFTPair.sol:437
NFTPair.sol:527
NFTPair.sol:674

TOOLS USED

Manual Analysis

MITIGATION

change variable++ to ++variable.

Require instead of AND

IMPACT

Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:189:
require(
                params.duration >= cur.duration && params.valuation <= cur.valuation && params.annualInterestBPS <= cur.annualInterestBPS,
                "NFTPair: worse params"
            );
NFTPair.sol:284:
require(
            params.valuation == accepted.valuation &&
                params.duration <= accepted.duration &&
                params.annualInterestBPS >= accepted.annualInterestBPS,
            "NFTPair: bad params"
        );
NFTPair.sol:622:
require(callee != address(bentoBox) && callee != address(collateral) && callee != address(this), "NFTPair: can't call");

NFTPairWithOracle.sol

NFTPairWithOracle.sol:206:
require(
                params.duration >= cur.duration &&
                    params.valuation <= cur.valuation &&
                    params.annualInterestBPS <= cur.annualInterestBPS &&
                    params.ltvBPS <= cur.ltvBPS,
                "NFTPair: worse params"
            );
NFTPairWithOracle.sol:312:
require(
            params.valuation == accepted.valuation &&
                params.duration <= accepted.duration &&
                params.annualInterestBPS >= accepted.annualInterestBPS &&
                params.ltvBPS >= accepted.ltvBPS,
            "NFTPair: bad params"
        );
NFTPairWithOracle.sol:655:
require(callee != address(bentoBox) && callee != address(collateral) && callee != address(this), "NFTPair: can't call");

TOOLS USED

Manual Analysis

MITIGATION

Instead of using &&, break down the conditions in multiple requirements statements (using custom errors would be even more gas efficient, see here)

-require(callee != address(bentoBox) && callee != address(collateral) && callee != address(this), "NFTPair: can't call");
+require(callee != address(bentoBox);
+require(callee != address(collateral)); +require(callee != address(this), "NFTPair: can't call"));

Tight Variable Packing

PROBLEM

Solidity contracts have contiguous 32 bytes (256 bits) slots used in storage. By arranging the variables, it is possible to minimize the number of slots used within a contract's storage and therefore reduce deployment costs.

Address type variables are each of 20 bytes size (way less than 32 bytes). However, they here take up a whole 32 bytes slot (they are contiguous).

As uint8 type variables are of size 1 byte, there's a slot here that can get saved by moving them closer to an address

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:80:
address public feeTo;

// Per clone variables
// Clone init settings
IERC721 public collateral;
IERC20 public asset;

// A note on terminology:
// "Shares" are BentoBox shares.

// Track assets we own. Used to allow skimming the excesss.
uint256 public feesEarnedShare;

// Per token settings.
mapping(uint256 => TokenLoanParams) public tokenLoanParams;

uint8 private constant LOAN_INITIAL = 0;
uint8 private constant LOAN_REQUESTED = 1;
uint8 private constant LOAN_OUTSTANDING = 2;

NFTPairWithOracle.sol

NFTPairWithOracle.sol:80:
address public feeTo;

// Per clone variables
// Clone init settings
IERC721 public collateral;
IERC20 public asset;

// A note on terminology:
// "Shares" are BentoBox shares.

// Track assets we own. Used to allow skimming the excesss.
uint256 public feesEarnedShare;

// Per token settings.
mapping(uint256 => TokenLoanParams) public tokenLoanParams;

uint8 private constant LOAN_INITIAL = 0;
uint8 private constant LOAN_REQUESTED = 1;
uint8 private constant LOAN_OUTSTANDING = 2;

TOOLS USED

Manual Analysis

MITIGATION

Place feeTo just before LOAN_INITIAL to save one storage slot

mapping(uint256 => TokenLoanParams) public tokenLoanParams;
+address public feeTo;
uint8 private constant LOAN_INITIAL = 0;
uint8 private constant LOAN_REQUESTED = 1;
uint8 private constant LOAN_OUTSTANDING = 2;

Unchecked arithmetic

IMPACT

The default "checked" behavior costs more gas when adding/diving/multiplying, because under-the-hood those checks are implemented as a series of opcodes that, prior to performing the actual arithmetic, check for under/overflow and revert if it is detected.

if it can statically be determined there is no possible way for your arithmetic to under/overflow (such as a condition in an if statement), surrounding the arithmetic in an unchecked block will save gas

PROOF OF CONCEPT

Instances include:

NFTPair.sol

NFTPair.sol:304: openFeeShare bounded by OPEN_FEE_BPS, overflow check unnecessary
NFTPair.sol:494: k bounded by COMPOUND_INTEREST_TERMS, overflow check unnecessary
NFTPair.sol:495: denom_k bounded by (COMPOUND_INTEREST_TERMS - 1)*YEAR_BPS , overflow check unnecessary
NFTPair.sol:641: i bounded by actions.length, overflow check unnecessary

NFTPairWithOracle.sol

NFTPairWithOracle.sol:339: openFeeShare bounded by OPEN_FEE_BPS, overflow check unnecessary
NFTPairWithOracle.sol:527: k bounded by COMPOUND_INTEREST_TERMS, overflow check unnecessary
NFTPairWithOracle.sol:528: denom_k bounded by (COMPOUND_INTEREST_TERMS - 1)*YEAR_BPS , overflow check unnecessary
NFTPairWithOracle.sol:674: i bounded by actions.length, overflow check unnecessary

TOOLS USED

Manual Analysis

MITIGATION

Place the arithmetic operations in an unchecked block