Blur Exchange contest - __141345__'s results

for-loop array.length should not be looked up in every loop

even for memory variables. The demo of the loop gas comparison can be seen here.

Instances number of this issue: 4

// contracts/BlurExchange.sol
199:    for (uint8 i = 0; i < orders.length; i++) {
476:    for (uint8 i = 0; i < fees.length; i++) {

// contracts/lib/EIP712.sol
77:     for (uint256 i = 0; i < fees.length; i++) {

// contracts/lib/MerkleVerifier.sol
38:     for (uint256 i = 0; i < proof.length; i++) {

The demo of the loop gas comparison can be seen here.

++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)

Saves 5 gas per loop

Instances number of this issue: 5

// contracts/BlurExchange.sol
199:    for (uint8 i = 0; i < orders.length; i++) {
476:    for (uint8 i = 0; i < fees.length; i++) {

// contracts/PolicyManager.sol
77:     for (uint256 i = 0; i < length; i++) {

// contracts/lib/EIP712.sol
77:     for (uint256 i = 0; i < fees.length; i++) {

// contracts/lib/MerkleVerifier.sol
38:     for (uint256 i = 0; i < proof.length; i++) {

++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops

This saves 30-40 gas per loop

Instances number of this issue: 5

// contracts/BlurExchange.sol
199:    for (uint8 i = 0; i < orders.length; i++) {
476:    for (uint8 i = 0; i < fees.length; i++) {

// contracts/PolicyManager.sol
77:     for (uint256 i = 0; i < length; i++) {

// contracts/lib/EIP712.sol
77:     for (uint256 i = 0; i < fees.length; i++) {

// contracts/lib/MerkleVerifier.sol
38:     for (uint256 i = 0; i < proof.length; i++) {

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.

Instances number of this issue: 6

// contracts/BlurExchange.sol
199:    for (uint8 i = 0; i < orders.length; i++) {
475:    uint256 totalFee = 0;
476:    for (uint8 i = 0; i < fees.length; i++) {

// contracts/PolicyManager.sol
77:     for (uint256 i = 0; i < length; i++) {

// contracts/lib/EIP712.sol
77:     for (uint256 i = 0; i < fees.length; i++) {

// contracts/lib/MerkleVerifier.sol
38:     for (uint256 i = 0; i < proof.length; i++) {

Usage of uint/int smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed

Reference: Layout of State Variables in Storage

Instances number of this issue: 2

// contracts/BlurExchange.sol
199:    for (uint8 i = 0; i < orders.length; i++) {
476:    for (uint8 i = 0; i < fees.length; i++) {

Expressions for constant values such as a call to keccak256(),` should use immutable rather than constant

Constant expressions are left as expressions, not constants.

Instances number of this issue: 5 When a constant declared as:

// 
20-35:
    bytes32 constant public FEE_TYPEHASH = keccak256(
        "Fee(uint16 rate,address recipient)"
    );
    bytes32 constant public ORDER_TYPEHASH = keccak256(
        "Order(address trader,uint8 side,address matchingPolicy,address collection,uint256 tokenId,uint256 amount,address paymentToken,uint256 price,uint256 listingTime,uint256 expirationTime,Fee[] fees,uint256 salt,bytes extraParams,uint256 nonce)Fee(uint16 rate,address recipient)"
    );
    bytes32 constant public ORACLE_ORDER_TYPEHASH = keccak256(
        "OracleOrder(Order order,uint256 blockNumber)Fee(uint16 rate,address recipient)Order(address trader,uint8 side,address matchingPolicy,address collection,uint256 tokenId,uint256 amount,address paymentToken,uint256 price,uint256 listingTime,uint256 expirationTime,Fee[] fees,uint256 salt,bytes extraParams,uint256 nonce)"
    );
    bytes32 constant public ROOT_TYPEHASH = keccak256(
        "Root(bytes32 root)"
    );

    bytes32 constant EIP712DOMAIN_TYPEHASH = keccak256(
        "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
    );

It is expected that the value should be converted into a constant value at compile time. But the expression is re-calculated each time the constant is referenced.

consequences:

• each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..)
• since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

Suggestion: Change these expressions from constant to immutable and implement the calculation in the constructor or hardcode these values in the constants and add a comment to say how the value was calculated.

reference: https://github.com/ethereum/solidity/issues/9232

Use private rather than public for constants

If needed, the values can be read from the verified contract source code, or if there are multiple values there can be a single getter function that returns a tuple of the values of all currently-public constants. Saves 3406-3606 gas in deployment gas due to the compiler not having to create non-payable getter functions for deployment calldata, not having to store the bytes of the value outside of where it's used, and not adding another entry to the method ID table

Instances number of this issue: 1

// contracts/BlurExchange.sol
59:     uint256 public constant INVERSE_BASIS_POINT = 10000;

X = X + Y / X = X - Y IS CHEAPER THAN X += Y / X -= Y

The demo of the gas comparison can be seen here.

Consider use X = X + Y / X = X - Y to save gas.

Instances number of this issue: 2

// contracts/BlurExchange.sol
208:    nonces[msg.sender] += 1;
479:    totalFee += fee;

internal functions only called once can be inlined

Not inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.

Instances number of this issue: 14

// contracts/BlurExchange.sol
278-281
    function _canSettleOrder(uint256 listingTime, uint256 expirationTime)
        view
        internal
        returns (bool)
344-352
    function _validateUserAuthorization() internal view returns (bool) {
375-380
    function _validateOracleAuthorization(
        bytes32 orderHash,
        SignatureVersion signatureVersion,
        bytes calldata extraSignature,
        uint256 blockNumber
    ) internal view returns (bool) {
416
    function _canMatchOrders(Order calldata sell, Order calldata buy)
444-450
    function _executeFundsTransfer(
        address seller,
        address buyer,
        address paymentToken,
        Fee[] calldata fees,
        uint256 price
    ) internal {
469-474
    function _transferFees(
        Fee[] calldata fees,
        address paymentToken,
        address from,
        uint256 price
    ) internal returns (uint256) {
525-532
    function _executeTokenTransfer(
        address collection,
        address from,
        address to,
        uint256 tokenId,
        uint256 amount,
        AssetType assetType
    ) internal {
548-551
    function _exists(address what)
        internal
        view
        returns (bool)

// contracts/lib/EIP712.sol
39-42
    function _hashDomain(EIP712Domain memory eip712Domain)
        internal
        pure
        returns (bytes32)
55-58
    function _hashFee(Fee calldata fee)
        internal 
        pure
        returns (bytes32)
69-72
    function _packFees(Fee[] calldata fees)
        internal
        pure
        returns (bytes32)
112-115
    function _hashToSign(bytes32 orderHash)
        internal
        view
        returns (bytes32 hash)
124-127
    function _hashToSignRoot(bytes32 root)
        internal
        view
        returns (bytes32 hash)
139-142
    function _hashToSignOracle(bytes32 orderHash, uint256 blockNumber)
        internal
        view
        returns (bytes32 hash)

Add unchecked {} for subtractions where the operands cannot underflow

Some will not underflow because of a previous code or require() or if-statement

    require(a <= b); 
    x = b - a 

->

    require(a <= b); 
    unchecked { x = b - a } 

Instances number of this issue: 2

// contracts/BlurExchange.sol
482-485
        require(totalFee <= price, "Total amount of fees are more than the price");

        uint256 receiveAmount = price - totalFee;

// contracts/PolicyManager.sol
71-73
        if (length > _whitelistedPolicies.length() - cursor) {
            length = _whitelistedPolicies.length() - cursor;
        }

require()/revert() strings longer than 32 bytes cost extra gas

Each extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas.

Instances number of this issue: 2

// contracts/BlurExchange.sol
482:    require(totalFee <= price, "Total amount of fees are more than the price");

// contracts/ExecutionDelegate.sol
22:    require(contracts[msg.sender], "Contract is not approved to make transfers");

Use custom errors rather than revert()/require() strings

Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas.

The demo of the gas comparison can be seen here.

Instances number of this issue: 26

// contracts/BlurExchange.sol
36:     require(isOpen == 1, "Closed");
134:    require(sell.order.side == Side.Sell);
139:    require(_validateOrderParameters(sell.order, sellHash), "Sell has invalid parameters");
140:    require(_validateOrderParameters(buy.order, buyHash), "Buy has invalid parameters");
142:    require(_validateSignatures(sell, sellHash), "Sell failed authorization");
143:    require(_validateSignatures(buy, buyHash), "Buy failed authorization");
183:    require(msg.sender == order.trader);
219:    require(address(_executionDelegate) != address(0), "Address cannot be zero");
228:    require(address(_policyManager) != address(0), "Address cannot be zero");
237:    require(_oracle != address(0), "Address cannot be zero");
318::    require(block.number - order.blockNumber < blockRange, "Signed block number out of range");
407:    require(v == 27 || v == 28, "Invalid v parameter");
424:    require(policyManager.isPolicyWhitelisted(sell.matchingPolicy), "Policy is not whitelisted");
428 require(policyManager.isPolicyWhitelisted(buy.matchingPolicy), "Policy is not whitelisted");
431:    require(canMatch, "Orders cannot be matched");
452:    require(msg.value == price);
482:    require(totalFee <= price, "Total amount of fees are more than the price");
534:    require(_exists(collection), "Collection does not exist");

// contracts/ExecutionDelegate.sol
22:     require(contracts[msg.sender], "Contract is not approved to make transfers");
77:     require(revokedApproval[from] == false, "User has revoked approval");
92:     require(revokedApproval[from] == false, "User has revoked approval");
108:    require(revokedApproval[from] == false, "User has revoked approval");
124:    require(revokedApproval[from] == false, "User has revoked approval");


// contracts/PolicyManager.sol
26:     require(!_whitelistedPolicies.contains(policy), "Already whitelisted");
37:     require(_whitelistedPolicies.contains(policy), "Not whitelisted");
// contracts/lib/ReentrancyGuarded.sol
14:     require(!reentrancyLock, "Reentrancy detected");

Duplicated require()/assert() checks could be refactored to a modifier or function

Instances number of this issue: 13

// contracts/BlurExchange.sol
139:    require(_validateOrderParameters(sell.order, sellHash), "Sell has invalid parameters");
140:    require(_validateOrderParameters(buy.order, buyHash), "Buy has invalid parameters");

142:    require(_validateSignatures(sell, sellHash), "Sell failed authorization");
143:    require(_validateSignatures(buy, buyHash), "Buy failed authorization");

219:    require(address(_executionDelegate) != address(0), "Address cannot be zero");
228:    require(address(_policyManager) != address(0), "Address cannot be zero");
237:    require(_oracle != address(0), "Address cannot be zero");

424:    require(policyManager.isPolicyWhitelisted(sell.matchingPolicy), "Policy is not whitelisted");
428:    require(policyManager.isPolicyWhitelisted(buy.matchingPolicy), "Policy is not whitelisted");

// contracts/ExecutionDelegate.sol
77:     require(revokedApproval[from] == false, "User has revoked approval");
92:     require(revokedApproval[from] == false, "User has revoked approval");
108:    require(revokedApproval[from] == false, "User has revoked approval");
124:    require(revokedApproval[from] == false, "User has revoked approval");

Boolean comparison

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value.

The demo of the gas comparison can be seen here.

Instances number of this issue: 5

// contracts/BlurExchange.sol
267:    (cancelledOrFilled[orderHash] == false) &&

// contracts/ExecutionDelegate.sol
77:     require(revokedApproval[from] == false, "User has revoked approval");
92:     require(revokedApproval[from] == false, "User has revoked approval");
108:    require(revokedApproval[from] == false, "User has revoked approval");
124:    require(revokedApproval[from] == false, "User has revoked approval");

Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner() is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.

The extra opcodes avoided are

CALLVALUE(2), DUP1(3), ISZERO(3), PUSH2(3), JUMPI(10), PUSH1(3), DUP1(3), REVERT(0), JUMPDEST(1), POP(2)

which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.

Instances number of this issue:

// 
43    function open() external onlyOwner {
47    function close() external onlyOwner {
53    function _authorizeUpgrade(address) internal override onlyOwner {}
215-217
    function setExecutionDelegate(IExecutionDelegate _executionDelegate)
        external
        onlyOwner
224-226
    function setPolicyManager(IPolicyManager _policyManager)
        external
        onlyOwner
233-235
    function setOracle(address _oracle)
        external
        onlyOwner
242-244
    function setBlockRange(uint256 _blockRange)
        external
        onlyOwner

// contracts/ExecutionDelegate.sol
36    function approveContract(address _contract) onlyOwner external {
45    function denyContract(address _contract) onlyOwner external {

// contracts/PolicyManager.sol
25    function addPolicy(address policy) external override onlyOwner {
36    function removePolicy(address policy) external override onlyOwner {

Not using the named return variables anywhere in the function is confusing

Consider changing the variable to be an unnamed one.

Instances number of this issue:

// contracts/BlurExchange.sol
416-419
    function _canMatchOrders(Order calldata sell, Order calldata buy)
        internal
        view
        returns (uint256 price, uint256 tokenId, uint256 amount, AssetType assetType)

// contracts/lib/EIP712.sol
112-115
    function _hashToSign(bytes32 orderHash)
        internal
        view
        returns (bytes32 hash)
124-127
    function _hashToSignRoot(bytes32 root)
        internal
        view
        returns (bytes32 hash)
139-142
    function _hashToSignOracle(bytes32 orderHash, uint256 blockNumber)
        internal
        view
        returns (bytes32 hash)

// contracts/lib/ERC1967Proxy.sol
29    function _implementation() internal view virtual override returns (address impl) {

Using bool for storage incurs overhead

// Booleans are more expensive than uint256 or any type that takes up a full

// word because each write operation emits an extra SLOAD to first read the
// slot's contents, replace the bits taken up by the boolean, and then write
// back. This is the compiler's defense against contract upgrades and
// pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27 Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past

Instances number of this issue:

// contracts/lib/ReentrancyGuarded.sol
10:     bool reentrancyLock = false;