AbraNFT contest - defsec's results

C4-001 :Use safeTransfer/safeTransferFrom consistently instead of transfer/transferFrom

Impact - LOW

Impact

It is good to add a require() statement that checks the return value of token transfers or to use something like OpenZeppelin’s safeTransfer/safeTransferFrom unless one is sure the given token reverts in case of a failure.

Proof of Concept

1. Navigate to the following contract.

2. transfer/transferFrom functions are used instead of safe transfer/transferFrom on the following contracts.

 https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L266

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L540

Tools Used

Code Review

Recommended Mitigation Steps

Consider using safeTransfer/safeTransferFrom or require() consistently. (https://docs.openzeppelin.com/contracts/2.x/api/token/erc721)

C4-002 : Use of ecrecover is susceptible to signature malleability

Impact

The ecrecover function is used in the contract to recover the address from the signature. The built-in EVM precompile ecrecover is susceptible to signature malleability which could lead to replay attacks (references: https://swcregistry.io/docs/SWC-117, https://swcregistry.io/docs/SWC-121 and https://medium.com/cryptronics/signature-replay-vulnerabilities-in-smart-contracts-3b6f7596df57).

Proof of Concept

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L383

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L419

Tools Used

None

Recommended Mitigation Steps

Consider using OpenZeppelin’s ECDSA library (which prevents this malleability) instead of the built-in function.

C4-003 : Loss Of Fees When Address Of feeTo is set To Zero Address

Impact

During the code review, It has been observed feeTo address is missing zero address. That will cause to fee lost.

Code Location

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L729

Tools Used

None

Recommended Mitigation Steps

Consider implementing zero address check.

C4-004 : Use of Block.timestamp

Impact - Non-Critical

Block timestamps have historically been used for a variety of applications, such as entropy for random numbers (see the Entropy Illusion for further details), locking funds for periods of time, and various state-changing conditional statements that are time-dependent. Miners have the ability to adjust timestamps slightly, which can prove to be dangerous if block timestamps are used incorrectly in smart contracts.

Proof of Concept

1. Navigate to the following contract.

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L511

Tools Used

Manual Code Review

Recommended Mitigation Steps

Block timestamps should not be used for entropy or generating random numbers—i.e., they should not be the deciding factor (either directly or through some derivation) for winning a game or changing an important state.

Time-sensitive logic is sometimes required; e.g., for unlocking contracts (time-locking), completing an ICO after a few weeks, or enforcing expiry dates. It is sometimes recommended to use block.number and an average block time to estimate times; with a 10 second block time, 1 week equates to approximately, 60480 blocks. Thus, specifying a block number at which to change a contract state can be more secure, as miners are unable to easily manipulate the block number.

C4-005 : Front-runnable Initializers

Impact - LOW

All contract initializers were missing access controls, allowing any user to initialize the contract. By front-running the contract deployers to initialize the contract, the incorrect parameters may be supplied, leaving the contract needing to be redeployed.

Proof of Concept

1. Navigate to the following contracts.

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L175

2. initialize functions does not have access control. They are vulnerable to front-running.

Tools Used

Manual Code Review

Recommended Mitigation Steps

While the code that can be run in contract constructors is limited, setting the owner in the contract's constructor to the msg.sender and adding the onlyOwner modifier to all initializers would be a sufficient level of access control.

C4-001: Revert String Size Optimization

Impact

Shortening revert strings to fit in 32 bytes will decrease deploy time gas and will decrease runtime gas when the revert condition has been met.

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

Proof of Concept

Revert strings > 32 bytes are here:

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L366

Tools Used

Manual Review

Recommended Mitigation Steps

Shorten the revert strings to fit in 32 bytes. That will affect gas optimization.

C4-002 : Cache array length in for loops can save gas

Impact

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.

Proof of Concept

1. Navigate to the following smart contract line.

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L641

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L674

Tools Used

None

Recommended Mitigation Steps

Consider to cache array length.

C4-003 : Non-strict inequalities are cheaper than strict ones

Impact

Strict inequalities add a check of non equality which costs around 3 gas.

Proof of Concept

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L739

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L717

Tools Used

Code Review

Recommended Mitigation Steps

Use >= or <= instead of > and < when possible.

C4-004 : There is no need to assign default values to variables

Impact - Gas Optimization

When a variable is declared solidity assigns the default value. In case the contract assigns the value again, it costs extra gas.

Example: uint x = 0 costs more gas than uint x without having any different functionality.

Proof of Concept

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L641

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L674

Tools Used

Code Review

Recommended Mitigation Steps

uint x = 0 costs more gas than uint x without having any different functionality.

C4-005 : Use Shift Right/Left instead of Division/Multiplication if possible

Impact

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.

While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.

Tools Used

None

Recommended Mitigation Steps

A division/multiplication by any number x being a power of 2 can be calculated by shifting log2(x) to the right/left.

C4-006: > 0 can be replaced with != 0 for gas optimization

Impact

!= 0 is a cheaper operation compared to > 0, when dealing with uint.

Proof of Concept

1. Navigate to the following contracts.

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L717

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L739

Tools Used

Code Review

Recommended Mitigation Steps

Use "!=0" instead of ">0" for the gas optimization.

C4-007 : Free gas savings for using solidity 0.8.10+

Impact

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free.

Proof of Concept

All Contracts

Solidity 0.8.10 has a useful change which reduced gas costs of external calls which expect a return value: https://blog.soliditylang.org/2021/11/09/solidity-0.8.10-release-announcement/

Code Generator: Skip existence check for external contract if return data is expected. In this case, the ABI decoder will revert if the contract does not exist

All Contracts

Tools Used

None

Recommended Mitigation Steps

Consider to upgrade pragma to at least 0.8.10.

C4-008 : ++i is more gas efficient than i++ in loops forwarding

Impact

++i is more gas efficient than i++ in loops forwarding.

Proof of Concept

1. Navigate to the following contracts.

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L641

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L674

Tools Used

Code Review

Recommended Mitigation Steps

It is recommend to use unchecked{++i} and change i declaration to uint256.

C4-009 : Using operator && used more gas

Impact

Using double require instead of operator && can save more gas.

Proof of Concept

1. Navigate to the following contracts.

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L206

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L189

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L284

Tools Used

Code Review

Recommended Mitigation Steps

Example

using &&:

function check(uint x)public view{
    require(x == 0 && x < 1 );
}
// gas cost 21630

using double require:

    require(x == 0 );
    require( x < 1);
    }
}
// gas cost 21622

C4-010 : Delete - ABI Coder V2 For Gas Optimization

Impact

From Pragma 0.8.0, ABI coder v2 is activated by default.

Proof of Concept

1. Navigate to the following code section.

""" https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPair.sol#L21

https://github.com/code-423n4/2022-04-abranft/blob/main/contracts/NFTPairWithOracle.sol#L21

"""

Tools Used

None

Recommended Mitigation Steps

ABI coder v2 is activated by default. It is recommended to delete redundant codes.

From Solidity v0.8.0 Breaking Changes https://docs.soliditylang.org/en/v0.8.0/080-breaking-changes.html