AbraNFT contest - oyc_109's results

missing checks for zero address

description

address(0) which is 20-bytes of 0’s is treated specially in Solidity contracts because the private key corresponding to this address is unknown.

Ether and tokens sent to this address cannot be retrieved and setting access control roles to this address also won’t work (no private key to sign transactions).

Therefore zero addresses should be used with care and checks should be implemented for user-supplied address parameters.

Findings

functions in NFTPair.sol do not check for zero addresses in parameters a require statement should be added for these functions

function _requestLoan
function removeCollateral
function _lend
function requestAndBorrow
function takeCollateralAndLend

do not calculate constants

Description

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

Example

NFTPair.sol

111: uint256 private constant YEAR_BPS = 3600 * 24 * 365 * 10_000;

Cache in variables instead of loading

Description

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

Example

NFTPair.sol

loan.status is used multiple times and should be cached

183: if (loan.status == LOAN_OUTSTANDING) {

params.duration is used multiple times and should be cached

189: params.duration >= cur.duration && params.valuation <= cur.valuation && params.annualInterestBPS <= cur.annualInterestBPS,

params.valuation is used multiple times and should be cached

189: params.duration >= cur.duration && params.valuation <= cur.valuation && params.annualInterestBPS <= cur.annualInterestBPS,

actions.length in the for loop should be cached

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

require statements should be checked first

Description

require statements can be placed earlier to reduce gas usage on revert

Example

NFTPair.sol

move lines to the top of their respective functions

186: require(msg.sender == loan.lender, "NFTPair: not the lender"); 
251: require(msg.sender == loan.borrower, "NFTPair: not the borrower");

switching between non-zero values saves gas

Description

SSTORE from 0 to 1 (or any non-zero value), the cost is 20000; SSTORE from 1 to 2 (or any other non-zero value), the cost is 5000.

Therefore, switching between 1, 2 instead of 0, 1 will be more gas efficient.

Example

in NFTPair.sol

change

96: uint8 private constant LOAN_INITIAL = 0;
97: uint8 private constant LOAN_REQUESTED = 1;
98: uint8 private constant LOAN_OUTSTANDING = 2;

to

96: uint8 private constant LOAN_INITIAL = 1;
97: uint8 private constant LOAN_REQUESTED = 2;
98: uint8 private constant LOAN_OUTSTANDING = 3;

don't cache values only used once

Description

values used only once should be calculated inline instead to save gas

Example

NFTPair.sol

304: uint256 borrowerShare = totalShare - openFeeShare;
520: uint256 fee = (interest * PROTOCOL_FEE_BPS) / BPS;

Prefix increments are cheaper than postfix increments

Description

using prefix increments save gas

Example

NFTPair.sol

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

use ++k and ++i instead

Don't Initialize Variables with Default Value

Description

saves gas

Example

NFTPair.sol

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

Use != 0 instead of > 0 for Unsigned Integer Comparison

Impact

Issue Information: G003 - use !=0 for unsigned int comparison

Example

NFTPair.sol::717 => if (_share > 0) {

Long Revert Strings

Impact

Issue Information: G007 - long (revert) strings

Example:

NFTPair.sol::366 => require(ILendingClub(lender).willLend(tokenId, params), "NFTPair: LendingClub does not like you");