Platform: Code4rena
Start Date: 05/10/2022
Pot Size: $50,000 USDC
Total HM: 2
Participants: 80
Period: 5 days
Judge: GalloDaSballo
Id: 168
League: ETH
Rank: 16/80
Findings: 2
Award: $1,239.39
🌟 Selected for report: 1
🚀 Solo Findings: 0
🌟 Selected for report: 0x4non
Also found by: 0x1f8b, 0xNazgul, 0xSmartContract, Deivitto, IllIllI, Lambda, RaymondFam, Rolezn, RustyRabbit, Trust, arcoun, bin2chen, brgltd, csanuragjain, d3e4, enckrish, exd0tpy, ladboy233, nicobevi, rbserver, rotcivegaf, simon135, zzykxx
416.821 USDC - $416.82
| Issue | Instances | |
|---|---|---|
| [L‑01] | Upgradeable contract not initialized | 1 |
| [L‑02] | Unsafe use of transfer()/transferFrom() with IERC20 | 1 |
| [L‑03] | Return values of transfer()/transferFrom() not checked | 1 |
| [L‑04] | require() should be used instead of assert() | 1 |
| [L‑05] | Missing checks for address(0x0) when assigning values to address state variables | 1 |
| [L‑06] | abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() | 1 |
| [L‑07] | Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions | 1 |
Total: 7 instances over 7 issues
| Issue | Instances | |
|---|---|---|
| [N‑01] | require()/revert() statements should have descriptive reason strings | 3 |
| [N‑02] | public functions not called by the contract should be declared external instead | 2 |
| [N‑03] | Non-assembly method available | 1 |
| [N‑04] | constants should be defined rather than using magic numbers | 6 |
| [N‑05] | Expressions for constant values such as a call to keccak256(), should use immutable rather than constant | 5 |
| [N‑06] | Lines are too long | 2 |
| [N‑07] | Variable names that consist of all capital letters should be reserved for constant/immutable variables | 1 |
| [N‑08] | File is missing NatSpec | 1 |
| [N‑09] | NatSpec is incomplete | 15 |
| [N‑10] | Event is missing indexed fields | 7 |
| [N‑11] | Not using the named return variables anywhere in the function is confusing | 4 |
| [N‑12] | Duplicated require()/revert() checks should be refactored to a modifier or function | 1 |
Total: 48 instances over 12 issues
Upgradeable contracts are initialized via an initializer function rather than by a constructor. Leaving such a contract uninitialized may lead to it being taken over by a malicious user
There is 1 instance of this issue:
File: contracts/BlurExchange.sol /// @audit missing __UUPS_init() 30: contract BlurExchange is IBlurExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, UUPSUpgradeable {
transfer()/transferFrom() with IERC20Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions on L1 do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert (see this link for a test case). Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead
While this contract is currently only used with WETH, there's nothing preventing it from being used with other tokens in other contexts
There is 1 instance of this issue:
File: contracts/ExecutionDelegate.sol 125: return IERC20(token).transferFrom(from, to, amount);
transfer()/transferFrom() not checkedNot all IERC20 implementations revert() when there's a failure in transfer()/transferFrom(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually making a payment
While this contract is currently only used with WETH, there's nothing preventing it from being used with other tokens in other contexts
There is 1 instance of this issue:
File: contracts/ExecutionDelegate.sol 125: return IERC20(token).transferFrom(from, to, amount);
require() should be used instead of assert()Prior to solidity version 0.8.0, hitting an assert consumes the remainder of the transaction's available gas rather than returning it, as require()/revert() do. assert() should be avoided even past solidity version 0.8.0 as its documentation states that "The assert function creates an error of type Panic(uint256). ... Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix".
There is 1 instance of this issue:
File: contracts/lib/ERC1967Proxy.sol 22: assert(_IMPLEMENTATION_SLOT == bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1));
address(0x0) when assigning values to address state variablesThere is 1 instance of this issue:
File: contracts/BlurExchange.sol 113: weth = _weth;
abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
If all arguments are strings and or bytes, bytes.concat() should be used instead
There is 1 instance of this issue:
File: contracts/lib/EIP712.sol 80: return keccak256(abi.encodePacked(feeHashes));
__gap[50] storage variable to allow for new storage variables in later versionsSee this link for a description of this storage variable. While some contracts may not currently be sub-classed, adding the variable now protects against forgetting to add it in the future.
There is 1 instance of this issue:
File: contracts/BlurExchange.sol 30: contract BlurExchange is IBlurExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, UUPSUpgradeable {
require()/revert() statements should have descriptive reason stringsThere are 3 instances of this issue:
File: contracts/BlurExchange.sol 134: require(sell.order.side == Side.Sell); 183: require(msg.sender == order.trader); 452: require(msg.value == price);
public functions not called by the contract should be declared external insteadContracts are allowed to override their parents' functions and change the visibility from external to public.
There are 2 instances of this issue:
File: contracts/BlurExchange.sol 95 function initialize( 96 uint chainId, 97 address _weth, 98 IExecutionDelegate _executionDelegate, 99 IPolicyManager _policyManager, 100 address _oracle, 101 uint _blockRange 102: ) public initializer {
File: contracts/lib/MerkleVerifier.sol 17 function _verifyProof( 18 bytes32 leaf, 19 bytes32 root, 20: bytes32[] memory proof
assembly{ id := chainid() } => uint256 id = block.chainid, assembly { size := extcodesize() } => uint256 size = address().code.length
There are some automated tools that will flag a project as having higher complexity if there is inline-assembly, so it's best to avoid using it where it's not necessary
There is 1 instance of this issue:
File: contracts/BlurExchange.sol 555: size := extcodesize(what)
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 6 instances of this issue:
File: contracts/BlurExchange.sol /// @audit 27 /// @audit 28 407: require(v == 27 || v == 28, "Invalid v parameter");
File: contracts/lib/MerkleVerifier.sol /// @audit 0x00 54: mstore(0x00, a) /// @audit 0x20 55: mstore(0x20, b) /// @audit 0x00 /// @audit 0x40 56: value := keccak256(0x00, 0x40)
keccak256(), should use immutable rather than constantWhile it doesn't save any gas because the compiler knows that developers often make this mistake, it's still best to use the right tool for the task at hand. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.
There are 5 instances of this issue:
File: contracts/lib/EIP712.sol 20 bytes32 constant public FEE_TYPEHASH = keccak256( 21 "Fee(uint16 rate,address recipient)" 22: ); 23 bytes32 constant public ORDER_TYPEHASH = keccak256( 24 "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)" 25: ); 26 bytes32 constant public ORACLE_ORDER_TYPEHASH = keccak256( 27 "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)" 28: ); 29 bytes32 constant public ROOT_TYPEHASH = keccak256( 30 "Root(bytes32 root)" 31: ); 33 bytes32 constant EIP712DOMAIN_TYPEHASH = keccak256( 34 "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)" 35: );
Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length
There are 2 instances of this issue:
File: contracts/lib/EIP712.sol 24: "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)" 27: "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)"
constant/immutable variablesIf the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).
There is 1 instance of this issue:
File: contracts/lib/EIP712.sol 37: bytes32 DOMAIN_SEPARATOR;
There is 1 instance of this issue:
File: contracts/lib/OrderStructs.sol
There are 15 instances of this issue:
File: contracts/BlurExchange.sol /// @audit Missing: '@return' 256 * @param orderHash hash of order 257 */ 258 function _validateOrderParameters(Order calldata order, bytes32 orderHash) 259 internal 260 view 261: returns (bool) /// @audit Missing: '@return' 276 * @param expirationTime order expiration time 277 */ 278 function _canSettleOrder(uint256 listingTime, uint256 expirationTime) 279 view 280 internal 281: returns (bool) /// @audit Missing: '@return' 289 * @param orderHash hash of order 290 */ 291 function _validateSignatures(Input calldata order, bytes32 orderHash) 292 internal 293 view 294: returns (bool) /// @audit Missing: '@return' 342 * @param extraSignature packed merkle path 343 */ 344 function _validateUserAuthorization( 345 bytes32 orderHash, 346 address trader, 347 uint8 v, 348 bytes32 r, 349 bytes32 s, 350 SignatureVersion signatureVersion, 351 bytes calldata extraSignature 352: ) internal view returns (bool) { /// @audit Missing: '@return' 373 * @param blockNumber block number used in oracle signature 374 */ 375 function _validateOracleAuthorization( 376 bytes32 orderHash, 377 SignatureVersion signatureVersion, 378 bytes calldata extraSignature, 379 uint256 blockNumber 380: ) internal view returns (bool) { /// @audit Missing: '@param digest' /// @audit Missing: '@return' 395 /** 396 * @dev Wrapped ecrecover with safety check for v parameter 397 * @param v v 398 * @param r r 399 * @param s s 400 */ 401 function _recover( 402 bytes32 digest, 403 uint8 v, 404 bytes32 r, 405 bytes32 s 406: ) internal pure returns (address) { /// @audit Missing: '@return' 414 * @param buy buy order 415 */ 416 function _canMatchOrders(Order calldata sell, Order calldata buy) 417 internal 418 view 419: returns (uint256 price, uint256 tokenId, uint256 amount, AssetType assetType) /// @audit Missing: '@return' 467 * @param price price of token 468 */ 469 function _transferFees( 470 Fee[] calldata fees, 471 address paymentToken, 472 address from, 473 uint256 price 474: ) internal returns (uint256) { /// @audit Missing: '@param amount' 517 /** 518 * @dev Execute call through delegate proxy 519 * @param collection collection contract address 520 * @param from seller address 521 * @param to buyer address 522 * @param tokenId tokenId 523 * @param assetType asset type of the token 524 */ 525 function _executeTokenTransfer( 526 address collection, 527 address from, 528 address to, 529 uint256 tokenId, 530 uint256 amount, 531: AssetType assetType /// @audit Missing: '@return' 546 * @param what address to check 547 */ 548 function _exists(address what) 549 internal 550 view 551: returns (bool)
File: contracts/ExecutionDelegate.sol /// @audit Missing: '@return' 117 * @param amount amount 118 */ 119 function transferERC20(address token, address from, address to, uint256 amount) 120 approvedContract 121 external 122: returns (bool)
File: contracts/lib/MerkleVerifier.sol /// @audit Missing: '@return' 31 * @param proof proof 32 */ 33 function _computeRoot( 34 bytes32 leaf, 35 bytes32[] memory proof 36: ) public pure returns (bytes32) {
File: contracts/PolicyManager.sol /// @audit Missing: '@return' 45 * @param policy address of the policy to check 46 */ 47: function isPolicyWhitelisted(address policy) external view override returns (bool) { /// @audit Missing: '@return' 61 * @param size size 62 */ 63 function viewWhitelistedPolicies(uint256 cursor, uint256 size) 64 external 65 view 66 override 67: returns (address[] memory, uint256)
indexed fieldsIndex event fields make the field more quickly accessible to off-chain tools that parse events. However, note that each index field costs extra gas during emission, so it's not necessarily best to index the maximum allowed per event (three fields). Each event should use three indexed fields if there are three or more fields, and gas usage is not particularly of concern for the events in question. If there are fewer than three fields, all of the fields should be indexed.
There are 7 instances of this issue:
File: contracts/BlurExchange.sol 76 event OrdersMatched( 77 address indexed maker, 78 address indexed taker, 79 Order sell, 80 bytes32 sellHash, 81 Order buy, 82 bytes32 buyHash 83: ); 85: event OrderCancelled(bytes32 hash); 86: event NonceIncremented(address trader, uint256 newNonce); 88: event NewExecutionDelegate(IExecutionDelegate executionDelegate); 89: event NewPolicyManager(IPolicyManager policyManager); 90: event NewOracle(address oracle); 91: event NewBlockRange(uint256 blockRange);
Consider changing the variable to be an unnamed one
There are 4 instances of this issue:
File: contracts/lib/EIP712.sol /// @audit hash 112 function _hashToSign(bytes32 orderHash) 113 internal 114 view 115: returns (bytes32 hash) /// @audit hash 124 function _hashToSignRoot(bytes32 root) 125 internal 126 view 127: returns (bytes32 hash) /// @audit hash 139 function _hashToSignOracle(bytes32 orderHash, uint256 blockNumber) 140 internal 141 view 142: returns (bytes32 hash)
File: contracts/lib/ERC1967Proxy.sol /// @audit impl 29: function _implementation() internal view virtual override returns (address impl) {
require()/revert() checks should be refactored to a modifier or functionThe compiler will inline the function, which will avoid JUMP instructions usually associated with functions
There is 1 instance of this issue:
File: contracts/ExecutionDelegate.sol 92: require(revokedApproval[from] == false, "User has revoked approval");
#0 - GalloDaSballo
2022-10-23T20:24:48Z
[L‑01] | Upgradeable contract not initialized L
[L‑02] | Unsafe use of transfer()/transferFrom() with IERC20 TODO - M-01
[L‑03] | Return values of transfer()/transferFrom() not checked TODO - M-01
[L‑04] | require() should be used instead of assert() R
[L‑05] | Missing checks for address(0x0) when assigning values to address state variables L
[L‑06] | abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() Disagree with the specific example shown as you'd need to demonstrate a clash of the output of keccak256
[L‑07] | Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions L
[N‑01] | require()/revert() statements should have descriptive reason strings NC
[N‑02] | public functions not called by the contract should be declared external instead R
[N‑03] | Non-assembly method available R
[N‑04] | constants should be defined rather than using magic numbers Disagree with the specific instances shown (v, r) and the other look like false positives
[N‑05] | Expressions for constant values such as a call to keccak256(), should use immutable rather than constant This is invalid, see https://github.com/GalloDaSballo/Braindead-Gas-Savings
[N‑06] | Lines are too long NC
[N‑07] | Variable names that consist of all capital letters should be reserved for constant/immutable variables Disagree only because I believe this is the tradeOff with a unchangeable variable with upgradeable contract
[N‑08] | File is missing NatSpec && [N‑09] | NatSpec is incomplete NC
[N‑10] | Event is missing indexed fields Disagree with blanket statement (have awarded more specific advice about indexing one specific event, see other reports)
[N‑11] | Not using the named return variables anywhere in the function is confusing R
[N‑12] | Duplicated require()/revert() checks should be refactored to a modifier or function R
Pretty strong report, just a couple of things that could be improved, really good
#1 - GalloDaSballo
2022-11-07T19:11:01Z
3L 5R 3NC
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0xNazgul, 0xSmartContract, Aymen0909, Heuss, Lambda, Pheonix, RaymondFam, ReyAdmirado, Ruhum, Shinchan, Shishigami, __141345__, adriro, ajtra, c3phas, ch0bu, cryptostellar5, d3e4, enckrish, gogo, halden, lucacez, mcwildy, medikko, neko_nyaa, pedr02b2, pfapostol, ret2basic, rvierdiiev, saian, sakman, sakshamguruji
822.5694 USDC - $822.57
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [G‑01] | State variables that never change should be declared immutable or constant | 1 | 2097 |
| [G‑02] | Multiple address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriate | 1 | 8400 |
| [G‑03] | Structs can be packed into fewer storage slots | 1 | - |
| [G‑04] | Using calldata instead of memory for read-only arguments in external functions saves gas | 1 | 120 |
| [G‑05] | State variables should be cached in stack variables rather than re-reading them from storage | 1 | 97 |
| [G‑06] | The result of function calls should be cached rather than re-calling the function | 1 | 17 |
| [G‑07] | internal functions only called once can be inlined to save gas | 10 | 200 |
| [G‑08] | Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement | 1 | 85 |
| [G‑09] | <array>.length should not be looked up in every loop of a for-loop | 4 | 12 |
| [G‑10] | ++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 | 5 | 300 |
| [G‑11] | require()/revert() strings longer than 32 bytes cost extra gas | 2 | - |
| [G‑12] | Optimize names to save gas | 3 | 66 |
| [G‑13] | Using bools for storage incurs overhead | 4 | 51300 |
| [G‑14] | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) | 5 | 25 |
| [G‑15] | Using private rather than public for constants, saves gas | 7 | - |
| [G‑16] | Don't compare boolean expressions to boolean literals | 5 | 45 |
| [G‑17] | Use custom errors rather than revert()/require() strings to save gas | 23 | - |
| [G‑18] | Functions guaranteed to revert when called by normal users can be marked payable | 11 | 231 |
Total: 86 instances over 18 issues with 62,995 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values; deployment values are listed in the individual issue descriptions
immutable or constantAvoids a Gsset (20000 gas) in the constructor, and replaces the first access in each transaction (Gcoldsload - 2100 gas) and each access thereafter (Gwarmacces - 100 gas) with a PUSH32 (3 gas).
While it's not possible to use immutable because the contract is UUPS, the variable can be declared as a hard-coded constant and get the same gas savings
There is 1 instance of this issue:
File: /contracts/BlurExchange.sol 509 } else if (paymentToken == weth) { 510 /* Transfer funds in WETH. */ 511: executionDelegate.transferERC20(weth, from, to, amount);
address/ID mappings can be combined into a single mapping of an address/ID to a struct, where appropriateSaves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.
All functions that check contracts also check revokedApproval, which means if the modifier is changed to check both, both the ~42 gas and the ~2100 gas savings get applied. There are four instances of the approvedContract() modifier, so making such a change saves approximately 8,400 gas
There is 1 instance of this issue:
File: contracts/ExecutionDelegate.sol 18 mapping(address => bool) public contracts; 19: mapping(address => bool) public revokedApproval;
Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings
There is 1 instance of this issue:
File: contracts/lib/OrderStructs.sol /// @audit Variable ordering with 6 slots instead of the current 7: /// user-defined(32):order, bytes32(32):r, bytes32(32):s, bytes(32):extraSignature, uint256(32):blockNumber, uint8(1):v, uint8(1):signatureVersion 30 struct Input { 31 Order order; 32 uint8 v; 33 bytes32 r; 34 bytes32 s; 35 bytes extraSignature; 36 SignatureVersion signatureVersion; 37 uint256 blockNumber; 38: }
calldata instead of memory for read-only arguments in external functions saves gasWhen a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it's still valid for implementation contracs to use calldata arguments instead.
If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it's still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one
Note that I've also flagged instances where the function is public but can be marked as external since it's not called by the contract, and cases where a constructor is involved
There is 1 instance of this issue:
File: contracts/lib/MerkleVerifier.sol /// @audit proof 17 function _verifyProof( 18 bytes32 leaf, 19 bytes32 root, 20: bytes32[] memory proof
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There is 1 instance of this issue:
File: contracts/BlurExchange.sol /// @audit weth on line 509 511: executionDelegate.transferERC20(weth, from, to, amount);
The instances below point to the second+ call of the function within a single function
There is 1 instance of this issue:
File: contracts/PolicyManager.sol /// @audit _whitelistedPolicies.length() on line 71 72: length = _whitelistedPolicies.length() - cursor;
internal functions only called once can be inlined to save gasNot inlining costs 20 to 40 gas because of two extra JUMP instructions and additional stack operations needed for function calls.
There are 10 instances of this issue:
File: contracts/BlurExchange.sol 278 function _canSettleOrder(uint256 listingTime, uint256 expirationTime) 279 view 280 internal 281: returns (bool) 344 function _validateUserAuthorization( 345 bytes32 orderHash, 346 address trader, 347 uint8 v, 348 bytes32 r, 349 bytes32 s, 350 SignatureVersion signatureVersion, 351 bytes calldata extraSignature 352: ) internal view returns (bool) { 375 function _validateOracleAuthorization( 376 bytes32 orderHash, 377 SignatureVersion signatureVersion, 378 bytes calldata extraSignature, 379 uint256 blockNumber 380: ) internal view returns (bool) { 416 function _canMatchOrders(Order calldata sell, Order calldata buy) 417 internal 418 view 419: returns (uint256 price, uint256 tokenId, uint256 amount, AssetType assetType) 444 function _executeFundsTransfer( 445 address seller, 446 address buyer, 447 address paymentToken, 448 Fee[] calldata fees, 449: uint256 price 469 function _transferFees( 470 Fee[] calldata fees, 471 address paymentToken, 472 address from, 473 uint256 price 474: ) internal returns (uint256) { 525 function _executeTokenTransfer( 526 address collection, 527 address from, 528 address to, 529 uint256 tokenId, 530 uint256 amount, 531: AssetType assetType 548 function _exists(address what) 549 internal 550 view 551: returns (bool)
File: contracts/lib/EIP712.sol 55 function _hashFee(Fee calldata fee) 56 internal 57 pure 58: returns (bytes32) 69 function _packFees(Fee[] calldata fees) 70 internal 71 pure 72: returns (bytes32)
unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statementrequire(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }
There is 1 instance of this issue:
File: contracts/BlurExchange.sol /// @audit require() on line 482 485: uint256 receiveAmount = price - totalFee;
<array>.length should not be looked up in every loop of a for-loopThe overheads outlined below are PER LOOP, excluding the first loop
MLOAD (3 gas)CALLDATALOAD (3 gas)Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
There are 4 instances of this issue:
File: contracts/BlurExchange.sol 199: for (uint8 i = 0; i < orders.length; i++) { 476: for (uint8 i = 0; i < fees.length; i++) {
File: contracts/lib/EIP712.sol 77: for (uint256 i = 0; i < fees.length; i++) {
File: 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-loopsThe unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop
There are 5 instances of this issue:
File: contracts/BlurExchange.sol 199: for (uint8 i = 0; i < orders.length; i++) { 476: for (uint8 i = 0; i < fees.length; i++) {
File: contracts/lib/EIP712.sol 77: for (uint256 i = 0; i < fees.length; i++) {
File: contracts/lib/MerkleVerifier.sol 38: for (uint256 i = 0; i < proof.length; i++) {
File: contracts/PolicyManager.sol 77: for (uint256 i = 0; i < length; i++) {
require()/revert() strings longer than 32 bytes cost extra gasEach extra memory word of bytes past the original 32 incurs an MSTORE which costs 3 gas
There are 2 instances of this issue:
File: contracts/BlurExchange.sol 482: require(totalFee <= price, "Total amount of fees are more than the price");
File: contracts/ExecutionDelegate.sol 22: require(contracts[msg.sender], "Contract is not approved to make transfers");
public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted
There are 3 instances of this issue:
File: contracts/BlurExchange.sol /// @audit open(), close(), initialize(), execute(), cancelOrder(), cancelOrders(), incrementNonce(), setExecutionDelegate(), setPolicyManager(), setOracle(), setBlockRange() 30: contract BlurExchange is IBlurExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, UUPSUpgradeable {
File: contracts/ExecutionDelegate.sol /// @audit approveContract(), denyContract(), revokeApproval(), grantApproval(), transferERC721Unsafe(), transferERC721(), transferERC1155(), transferERC20() 16: contract ExecutionDelegate is IExecutionDelegate, Ownable {
File: contracts/lib/MerkleVerifier.sol /// @audit _verifyProof(), _computeRoot() 8: library MerkleVerifier {
bools 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
There are 4 instances of this issue:
File: contracts/BlurExchange.sol /// @audit excluding this one from the gas count since it's not changed after being set 71: mapping(bytes32 => bool) public cancelledOrFilled;
File: contracts/ExecutionDelegate.sol 18: mapping(address => bool) public contracts; 19: mapping(address => bool) public revokedApproval;
File: contracts/lib/ReentrancyGuarded.sol 10: bool reentrancyLock = false;
++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)Saves 5 gas per loop
There are 5 instances of this issue:
File: contracts/BlurExchange.sol 199: for (uint8 i = 0; i < orders.length; i++) { 476: for (uint8 i = 0; i < fees.length; i++) {
File: contracts/lib/EIP712.sol 77: for (uint256 i = 0; i < fees.length; i++) {
File: contracts/lib/MerkleVerifier.sol 38: for (uint256 i = 0; i < proof.length; i++) {
File: contracts/PolicyManager.sol 77: for (uint256 i = 0; i < length; i++) {
private rather than public for constants, saves gasIf 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
There are 7 instances of this issue:
File: contracts/BlurExchange.sol 57: string public constant name = "Blur Exchange"; 58: string public constant version = "1.0"; 59: uint256 public constant INVERSE_BASIS_POINT = 10000;
File: contracts/lib/EIP712.sol 20 bytes32 constant public FEE_TYPEHASH = keccak256( 21 "Fee(uint16 rate,address recipient)" 22: ); 23 bytes32 constant public ORDER_TYPEHASH = keccak256( 24 "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)" 25: ); 26 bytes32 constant public ORACLE_ORDER_TYPEHASH = keccak256( 27 "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)" 28: ); 29 bytes32 constant public ROOT_TYPEHASH = keccak256( 30 "Root(bytes32 root)" 31: );
if (<x> == true) => if (<x>), if (<x> == false) => if (!<x>)
There are 5 instances of this issue:
File: contracts/BlurExchange.sol 267: (cancelledOrFilled[orderHash] == false) &&
File: 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");
revert()/require() strings to save gasCustom 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
There are 23 instances of this issue:
File: contracts/BlurExchange.sol 36: require(isOpen == 1, "Closed"); 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"); 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"); 482: require(totalFee <= price, "Total amount of fees are more than the price"); 534: require(_exists(collection), "Collection does not exist");
File: 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");
File: contracts/lib/ReentrancyGuarded.sol 14: require(!reentrancyLock, "Reentrancy detected");
File: contracts/PolicyManager.sol 26: require(!_whitelistedPolicies.contains(policy), "Already whitelisted"); 37: require(_whitelistedPolicies.contains(policy), "Not whitelisted");
payableIf 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
There are 11 instances of this issue:
File: contracts/BlurExchange.sol 43: function open() external onlyOwner { 47: function close() external onlyOwner { 53: function _authorizeUpgrade(address) internal override onlyOwner {} 215 function setExecutionDelegate(IExecutionDelegate _executionDelegate) 216 external 217: onlyOwner 224 function setPolicyManager(IPolicyManager _policyManager) 225 external 226: onlyOwner 233 function setOracle(address _oracle) 234 external 235: onlyOwner 242 function setBlockRange(uint256 _blockRange) 243 external 244: onlyOwner
File: contracts/ExecutionDelegate.sol 36: function approveContract(address _contract) onlyOwner external { 45: function denyContract(address _contract) onlyOwner external {
File: contracts/PolicyManager.sol 25: function addPolicy(address policy) external override onlyOwner { 36: function removePolicy(address policy) external override onlyOwner {
#0 - GalloDaSballo
2022-10-23T00:48:22Z
2.1k from weth 500 memory 5k from reentancy
Rest is 150
Disagree with G-02 as they are meant to be separate
7750
#1 - GalloDaSballo
2022-11-03T20:54:41Z
Ultimately best submission as it contained both high leverage savings + some minor savings that did add up to win, wp