Blur Exchange contest - 0x1f8b's results

Lines of code

https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L36 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L93 https://github.com/code-423n4/2022-10-blur/blob/2fdaa6e13b544c8c11d1c022a575f16c3a72e3bf/contracts/ExecutionDelegate.sol#L109

Vulnerability details

Impact

The ExecutionDelegate contract is vulnerable to rogue attacks. The owner can approve any contract and call transferERC20, transferERC721 or transferERC1155 for steal tokens.

Proof of Concept

• Owner call approveContract(attaker)
• Attacker call transferERC721 and steal the tokens.

Recommended Mitigation Steps

• Use TimeLock for governance.

• Low
  • 1. Outdated packages
  • 2. Outdated compiler
  • 3. Allows malleable SECP256K1 signatures
  • 4. Lack of ACK during owner change
  • 5. Lack of checks
• Non critical
  • 6. Use abstract for base contracts
  • 7. Bad nomenclature
  • 8. Do the input validations during the validation phase

Low

1. Outdated packages

Some used packages are out of date, it is good practice to use the latest version of these packages:

"@openzeppelin/contracts": "4.4.1",
"@openzeppelin/contracts-upgradeable": "^4.6.0",

Reference:

• https://github.com/OpenZeppelin/openzeppelin-contracts/releases

2. Outdated compiler

The pragma version used are:

pragma solidity 0.8.13;

The minimum required version must be 0.8.16; otherwise, contracts will be affected by the following important bug fixes:

0.8.14:

• ABI Encoder: When ABI-encoding values from calldata that contain nested arrays, correctly validate the nested array length against calldatasize() in all cases.
• Override Checker: Allow changing data location for parameters only when overriding external functions.

0.8.15

• Code Generation: Avoid writing dirty bytes to storage when copying bytes arrays.
• Yul Optimizer: Keep all memory side-effects of inline assembly blocks.

0.8.16

• Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

Apart from these, there are several minor bug fixes and improvements.

3. Allows malleable SECP256K1 signatures

Here, the ecrecover() method doesn't check the s range.

Homestead (EIP-2) added this limitation, however the precompile remained unaltered. The majority of libraries, including OpenZeppelin, do this check.

Since an order can only be confirmed once and its hash is saved, there doesn't seem to be a serious danger in existing use cases.

Reference:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/7201e6707f6631d9499a569f492870ebdd4133cf/contracts/utils/cryptography/ECDSA.sol#L138-L149

Affected source code:

• BlurExchange.sol:408

4. Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because of human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

• BlurExchange.sol:30
• PolicyManager.sol:13
• ExecutionDelegate.sol:16

5. Lack of checks

The following methods have a lack of checks if the received argument is an address, it's good practice in order to reduce human error to check that the address specified in the constructor or initialize is different than address(0).

Ensure that it's a contract:

• BlurExchange.sol:97-100
• BlurExchange.sol:220
• BlurExchange.sol:229
• BlurExchange.sol:238

---

Non critical

6. Use abstract for base contracts

abstract contracts are contracts that have at least one function without its implementation. An instance of an abstract cannot be created.

Reference:

• https://docs.soliditylang.org/en/v0.6.2/contracts.html#abstract-contracts

Affected source code:

• ReentrancyGuarded.sol:8

7. Bad nomenclature

The _exists method, according to its comment, returns if an address exists, this is not real, it returns if an address is not an EOA, which is not the same, the comment and the name of said method should be modified.

/**
-       * @dev Determine if the given address exists
+       * @dev Determine if the given address is a contract
        * @param what address to check
        */
-   function _exists(address what)
+   function _isContract(address what)
        internal
        view
        returns (bool)
    {
        uint size;
        assembly {
            size := extcodesize(what)
        }
        return size > 0;
    }

Affected source code:

• BlurExchange.sol:544-558

8. Do the input validations during the validation phase

The paymentToken is checked during the _transferTo, it's better to transfer all the erc20, and do this check during _validateOrderParameters function.

    function _transferTo(
        address paymentToken,
        address from,
        address to,
        uint256 amount
    ) internal {
        if (amount == 0) {
            return;
        }

        if (paymentToken == address(0)) {
            /* Transfer funds in ETH. */
            payable(to).transfer(amount);
-       } else if (paymentToken == weth) {
-           /* Transfer funds in WETH. */
+       } else {
            executionDelegate.transferERC20(weth, from, to, amount);
-       } else {
-           revert("Invalid payment token");
        }
    }

Affected source code:

• BlurExchange.sol:509-514

• Gas
  • 1. Use require instead of assert
  • 2. Don't use the length of an array for loops condition
  • 3. Reduce boolean comparison
    • Total gas saved: 18 * 0 + 15 * 5 = 75
  • 4. Avoid compound assignment operator in state variables
    • Total gas saved: 13 * 2 = 26
  • 5. Remove empty blocks
  • 6. ++i costs less gas compared to i++ or i += 1
  • 7. Reduce the size of error messages Long revert Strings
    • Total gas saved: 18 * 3 = 54
  • 8. Use Custom Errors instead of Revert Strings to save Gas
    • Total gas saved: 9 * 26 = 234
  • 9. There's no need to set default values for variables
    • Total gas saved: 8 * 7 = 56
  • 10. Change bool to uint256 can save gas
  • 11. Remove unnecessary variables
  • 12. constants expressions are expressions, not constants
  • 13. Optimize EIP712._hashOrder
  • 14. Optimize PolicyManager.addPolicy
  • 15. Optimize PolicyManager.removePolicy
  • 16. Change bool to uint256 can save gas
  • 17. delete optimization
    • Total gas saved: 5 * 2 = 10
  • 18. Optimize BlurExchange._validateOracleAuthorization

Gas

1. Use require instead of assert

The assert() and require() functions are a part of the error handling aspect in Solidity. Solidity makes use of state-reverting error handling exceptions. This means all changes made to the contract on that call or any sub-calls are undone if an error is thrown. It also flags an error.

They are quite similar as both check for conditions and if they are not met, would throw an error.

The big difference between the two is that the assert() function when false, uses up all the remaining gas and reverts all the changes made.

Meanwhile, a require() function when false, also reverts back all the changes made to the contract but does refund all the remaining gas fees we offered to pay. This is the most common Solidity function used by developers for debugging and error handling.

Affected source code:

• ERC1967Proxy.sol:22

2. Don't use the length of an array for loops condition

It's cheaper to store the length of the array inside a local variable and iterate over it.

Affected source code:

• BlurExchange.sol:199
• BlurExchange.sol:476
• EIP712.sol:77
• MerkleVerifier.sol:38

3. Reduce boolean comparison

Compare a boolean value using == true or == false instead of using the boolean value is more expensive.

NOT opcode it's cheaper than using EQUAL or NOTEQUAL when the value it's false, or just the value without == true when it's true, because it will use less opcodes inside the VM.

Proof of concept (without optimizations):

pragma solidity 0.8.16;

contract TesterA {
function testEqual(bool a) public view returns (bool) { return a == true; }
}

contract TesterB {
function testNot(bool a) public view returns (bool) { return a; }
}

Gas saving executing: 18 per entry for == true

TesterA.testEqual:   21814
TesterB.testNot:     21796   

pragma solidity 0.8.16;

contract TesterA {
function testEqual(bool a) public view returns (bool) { return a == false; }
}

contract TesterB {
function testNot(bool a) public view returns (bool) { return !a; }
}

Gas saving executing: 15 per entry for == false

TesterA.testEqual:   21814
TesterB.testNot:     21799

Affected source code:

Use ! instead of == false:

• BlurExchange.sol:267
• ExecutionDelegate.sol:77
• ExecutionDelegate.sol:92
• ExecutionDelegate.sol:108
• ExecutionDelegate.sol:124

Total gas saved: (18 * 0) + (15 * 5) = 75

4. Avoid compound assignment operator in state variables

Using compound assignment operators for state variables (like State += X or State -= X ...) it's more expensive than using operator assignment (like State = State + X or State = State - X ...).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
uint private _a;
function testShort() public {
_a += 1;
}
}

contract TesterB {
uint private _a;
function testLong() public {
_a = _a + 1;
}
}

Gas saving executing: 13 per entry

TesterA.testShort: 43507
TesterB.testLong:  43494

Affected source code:

• BlurExchange.sol:208
• BlurExchange.sol:479

Total gas saved: 13 * 2 = 26

5. Remove empty blocks

An empty block or an empty method generally indicates a lack of logic that it’s unnecessary and should be eliminated, call a method that literally does nothing only consumes gas unnecessarily, if it is a virtual method which is expects it to be filled by the class that implements it, it is better to use abstract contracts with just the definition.

Sample code:

pragma solidity >=0.4.0 <0.7.0;

abstract contract BaseContract {
    function totalSupply() public virtual returns (uint256);
}

Reference:

• https://docs.soliditylang.org/en/v0.6.0/contracts.html?highlight=abstract#abstract-contracts

Affected source code:

• BlurExchange.sol:53

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

++i costs less gas compared to i++ or i += 1 for unsigned integers, 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 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Keep in mind that this change can only be made when we are not interested in the value returned by the operation, since the result is different, you only have to apply it when you only want to increase a counter.

Affected source code:

• BlurExchange.sol:199
• BlurExchange.sol:476
• PolicyManager.sol:77
• EIP712.sol:77
• MerkleVerifier.sol:38

7. 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.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testShortRevert(bool path) public view {
require(path, "test error");
}
}

contract TesterB {
function testLongRevert(bool path) public view {
require(path, "test big error message, more than 32 bytes");
}
}

Gas saving executing: 18 per entry

TesterA.testShortRevert: 21886
TesterB.testLongRevert:  21904

Affected source code:

• BlurExchange.sol:318
• BlurExchange.sol:482
• ExecutionDelegate.sol:22

Total gas saved: 18 * 3 = 54

8. Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testRevert(bool path) public view {
 require(path, "test error");
}
}

contract TesterB {
error MyError(string msg);
function testError(bool path) public view {
 if(path) revert MyError("test error");
}
}

Gas saving executing: 9 per entry

TesterA.testRevert: 21611
TesterB.testError:  21602     

Affected source code:

• BlurExchange.sol:36
• BlurExchange.sol:134
• BlurExchange.sol:139
• BlurExchange.sol:140
• BlurExchange.sol:142
• BlurExchange.sol:143
• BlurExchange.sol:183
• BlurExchange.sol:219
• BlurExchange.sol:228
• BlurExchange.sol:237
• BlurExchange.sol:318
• BlurExchange.sol:407
• BlurExchange.sol:424
• BlurExchange.sol:428
• BlurExchange.sol:431
• BlurExchange.sol:452
• BlurExchange.sol:482
• BlurExchange.sol:534
• ExecutionDelegate.sol:22
• ExecutionDelegate.sol:77
• ExecutionDelegate.sol:92
• ExecutionDelegate.sol:108
• ExecutionDelegate.sol:124
• PolicyManager.sol:26
• PolicyManager.sol:37
• ReentrancyGuarded.sol:14

Total gas saved: 9 * 26 = 234

9. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
function testInit() public view returns (uint) { uint a = 0; return a; }
}

contract TesterB {
function testNoInit() public view returns (uint) { uint a; return a; }
}

Gas saving executing: 8 per entry

TesterA.testInit:   21392
TesterB.testNoInit: 21384

Affected source code:

• BlurExchange.sol:475
• ReentrancyGuarded.sol:10
• MerkleVerifier.sol:38
• BlurExchange.sol:199
• BlurExchange.sol:476
• PolicyManager.sol:77
• EIP712.sol:77

Total gas saved: 8 * 7 = 56

10. Change bool to uint256 can save gas

Because each write operation requires an additional SLOAD to read the slot's contents, replace the bits occupied by the boolean, and then write back, booleans are more expensive than uint256 or any other type that uses a complete word. This cannot be turned off because it is the compiler's defense against pointer aliasing and contract upgrades.

Reference:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol#L23-L27

Also, this is applicable to integer types different from uint256 or int256.

Affected source code for booleans:

• ReentrancyGuarded.sol:10
• ExecutionDelegate.sol:18
• ExecutionDelegate.sol:19

11. Remove unnecessary variables

The following state variables can be removed without affecting the logic of the contract since they are not used and/or are redundant because they could be used inline.

Affected source code:

• Remove computedRoot in MerkleVerifier.sol:22
• Remove proofElement in MerkleVerifier.sol:39

12. constants expressions are expressions, not constants

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.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

Reference:

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

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).

Affected source code:

• EIP712.sol:20-35

13. Optimize EIP712._hashOrder

Remove the bytes.concat and change the method like this:

    function _hashOrder(Order calldata order, uint256 nonce)
        internal
        pure
        returns (bytes32)
    {
        return keccak256(
-           bytes.concat(
                abi.encode(
                      ORDER_TYPEHASH,
                      order.trader,
                      order.side,
                      order.matchingPolicy,
                      order.collection,
                      order.tokenId,
                      order.amount,
                      order.paymentToken,
                      order.price,
                      order.listingTime,
                      order.expirationTime,
                      _packFees(order.fees),
                      order.salt,
+                     keccak256(order.extraParams),
+                     nonce
-                     keccak256(order.extraParams)
-               ),
-               abi.encode(nonce)
            )
        );
    }

Affected source code:

• EIP712.sol:84-110

14. Optimize PolicyManager.addPolicy

The method add already checks if the element is already present in the AddressSet.

It's recommended to apply the following changes:

    function addPolicy(address policy) external override onlyOwner {
-       require(!_whitelistedPolicies.contains(policy), "Already whitelisted");
-       _whitelistedPolicies.add(policy);
+       require(_whitelistedPolicies.add(policy), "Already whitelisted");

        emit PolicyWhitelisted(policy);
    }

Affected source code:

• PolicyManager.sol:25-30

15. Optimize PolicyManager.removePolicy

The method remove already checks if the element is already present in the AddressSet.

It's recommended to apply the following changes:

    function removePolicy(address policy) external override onlyOwner {
+       require(_whitelistedPolicies.remove(policy), "Not whitelisted");
-       require(_whitelistedPolicies.contains(policy), "Not whitelisted");
-       _whitelistedPolicies.remove(policy);

        emit PolicyRemoved(policy);
    }

Affected source code:

• PolicyManager.sol:36-41

16. Change bool to uint256 can save gas

Because each write operation requires an additional SLOAD to read the slot's contents, replace the bits occupied by the boolean, and then write back, booleans are more expensive than uint256 or any other type that uses a complete word. This cannot be turned off because it is the compiler's defense against pointer aliasing and contract upgrades.

Reference:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol#L23-L27

Also, this is applicable to integer types different from uint256 or int56.

Affected source code for booleans:

• ExecutionDelegate.sol:18
• ExecutionDelegate.sol:19
• BlurExchange.sol:71

17. delete optimization

Use delete instead of set to default value (false or 0).

5 gas could be saved per entry in the following affected lines:

Affected source code:

• ExecutionDelegate.sol:46
• ExecutionDelegate.sol:62

Total gas saved: 5 * 2 = 10

18. Optimize BlurExchange._validateOracleAuthorization

If the Bulk signatures are stored like the single, but with the merklePath at the end, instead of at the begining, it could be easier to parse because it's not required to check the signatureVersion.

        uint8 v; bytes32 r; bytes32 s;
-       if (signatureVersion == SignatureVersion.Single) {
            (v, r, s) = abi.decode(extraSignature, (uint8, bytes32, bytes32));
-       } else if (signatureVersion == SignatureVersion.Bulk) {
-           /* If the signature was a bulk listing the merkle path musted be unpacked before the oracle signature. */
-           (bytes32[] memory merklePath, uint8 _v, bytes32 _r, bytes32 _s) = abi.decode(extraSignature, (bytes32[], uint8, bytes32, bytes32));
-           v = _v; r = _r; s = _s;
        }

Affected source code:

• BlurExchange.sol:383-390