Platform: Code4rena
Start Date: 13/01/2023
Pot Size: $100,500 USDC
Total HM: 1
Participants: 23
Period: 10 days
Judge: hickuphh3
Total Solo HM: 1
Id: 201
League: ETH
Rank: 2/23
Findings: 2
Award: $2,757.95
š Selected for report: 0
š Solo Findings: 0
š Selected for report: horsefacts
Also found by: 0xSmartContract, ABA, Chom, IllIllI, Josiah, RaymondFam, Rickard, Rolezn, brgltd, btk, chaduke, charlesjhongc, csanuragjain, delfin454000, nadin, oyc_109
1551.4544 USDC - $1,551.45
| Issue | Instances | |
|---|---|---|
| [Nā01] | Import declarations should import specific identifiers, rather than the whole file | 28 |
| [Nā02] | Adding a return statement when the function defines a named return variable, is redundant | 4 |
| [Nā03] | Non-assembly method available | 3 |
| [Nā04] | constants should be defined rather than using magic numbers | 45 |
| [Nā05] | Events that mark critical parameter changes should contain both the old and the new value | 1 |
| [Nā06] | Typos | 2 |
| [Nā07] | File is missing NatSpec | 10 |
| [Nā08] | NatSpec is incomplete | 2 |
| [Nā09] | Not using the named return variables anywhere in the function is confusing | 13 |
| [Nā10] | Large assembly blocks should have extensive comments | 6 |
| [Nā11] | Contracts should have full test coverage | 1 |
| [Nā12] | Function ordering does not follow the Solidity style guide | 1 |
| [Nā13] | Interfaces should be indicated with an I prefix in the contract name | 9 |
Total: 125 instances over 13 issues
Using import declarations of the form import {<identifier_name>} from "some/file.sol" avoids polluting the symbol namespace making flattened files smaller, and speeds up compilation
There are 28 instances of this issue:
File: contracts/conduit/Conduit.sol 15: import "./lib/ConduitConstants.sol";
File: contracts/lib/AmountDeriver.sol 8: import "./ConsiderationConstants.sol";
File: contracts/lib/Assertions.sol 14: import "./ConsiderationErrors.sol";
File: contracts/lib/BasicOrderFulfiller.sol 23: import "./ConsiderationErrors.sol";
File: contracts/lib/ConsiderationBase.sol 12: import "./ConsiderationConstants.sol";
File: contracts/lib/ConsiderationDecoder.sol 20: import "./ConsiderationConstants.sol"; 22: import "../helpers/PointerLibraries.sol";
File: contracts/lib/ConsiderationEncoder.sol 4: import "./ConsiderationConstants.sol"; 20: import "../helpers/PointerLibraries.sol";
File: contracts/lib/ConsiderationErrors.sol 6: import "./ConsiderationConstants.sol";
File: contracts/lib/Consideration.sol 23: import "../helpers/PointerLibraries.sol"; 25: import "./ConsiderationConstants.sol";
File: contracts/lib/CounterManager.sol 10: import "./ConsiderationConstants.sol";
File: contracts/lib/CriteriaResolution.sol 15: import "./ConsiderationErrors.sol"; 17: import "../helpers/PointerLibraries.sol";
File: contracts/lib/Executor.sol 16: import "./ConsiderationConstants.sol"; 18: import "./ConsiderationErrors.sol";
File: contracts/lib/FulfillmentApplier.sol 16: import "./ConsiderationErrors.sol";
File: contracts/lib/GettersAndDerivers.sol 8: import "./ConsiderationConstants.sol";
File: contracts/lib/LowLevelHelpers.sol 4: import "./ConsiderationConstants.sol";
File: contracts/lib/OrderCombiner.sol 23: import "./ConsiderationErrors.sol";
File: contracts/lib/OrderFulfiller.sol 23: import "./ConsiderationErrors.sol";
File: contracts/lib/OrderValidator.sol 19: import "./ConsiderationErrors.sol";
File: contracts/lib/ReentrancyGuard.sol 8: import "./ConsiderationErrors.sol";
File: contracts/lib/SignatureVerification.sol 12: import "./ConsiderationErrors.sol";
File: contracts/lib/TokenTransferrer.sol 4: import "./TokenTransferrerConstants.sol";
File: contracts/lib/Verifiers.sol 10: import "./ConsiderationErrors.sol";
File: contracts/lib/ZoneInteraction.sol 16: import "./ConsiderationEncoder.sol";
return statement when the function defines a named return variable, is redundantThere are 4 instances of this issue:
File: contracts/lib/AmountDeriver.sol 87: return amount;
File: contracts/lib/BasicOrderFulfiller.sol 902: return orderHash;
File: contracts/lib/FulfillmentApplier.sol 143: return execution; // Execution(considerationItem, offerer, conduitKey);
File: contracts/lib/OrderCombiner.sol 1047: return executions;
assembly{ id := chainid() } => uint256 id = block.chainid, assembly { size := extcodesize() } => uint256 size = address().code.length, assembly { hash := extcodehash() } => bytes32 hash = address().codehash
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 are 3 instances of this issue:
File: contracts/lib/ConsiderationBase.sol 110: mstore(EIP712_domainData_chainId_offset, chainid())
File: contracts/lib/TokenTransferrer.sol 274: if iszero(extcodesize(token)) { 414: if iszero(extcodesize(token)) {
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 45 instances of this issue:
File: contracts/lib/BasicOrderFulfiller.sol /// @audit 3 86: orderType := and(basicOrderType, 3) /// @audit 2 89: route := shr(2, basicOrderType) /// @audit 3 127: offerTypeIsAdditionalRecipientsType := gt(route, 3)
File: contracts/lib/ConsiderationConstants.sol /// @audit 4 244: address constant IdentityPrecompile = address(4);
File: contracts/lib/ConsiderationDecoder.sol /// @audit 0xffff 872: gt(or(offerLength, considerationLength), 0xffff),
File: contracts/lib/CounterManager.sol /// @audit 1 43: blockhash(sub(number(), 1))
File: contracts/lib/CriteriaResolution.sol /// @audit 3 /// @audit 4 229: newItemType := sub(3, eq(itemType, 4)) /// @audit 3 253: withCriteria := gt(itemType, 3)
File: contracts/lib/FulfillmentApplier.sol /// @audit 1 326: shl(1, lt(newAmount, amount)), /// @audit 1 437: if eq(errorBuffer, 1) { /// @audit 1 601: shl(1, lt(newAmount, amount)), /// @audit 1 701: if eq(errorBuffer, 1) {
File: contracts/lib/OrderCombiner.sol /// @audit 32 220: terminalMemoryOffset = (totalOrders + 1) * 32; /// @audit 32 /// @audit 32 230: for (uint256 i = 32; i < terminalMemoryOffset; i += 32) { /// @audit 4 292: let isNonContract := lt(orderType, 4) /// @audit 32 /// @audit 32 451: for (uint256 i = 32; i < terminalMemoryOffset; i += 32) {
File: contracts/lib/OrderFulfiller.sol /// @audit 4 264: lt(orderType, 4),
File: contracts/lib/OrderValidator.sol /// @audit 1 157: invalidFraction := xor(mul(numerator, denominator), 1) /// @audit 1 237: } 1 { /// @audit 1 253: if eq(denominator, 1) { /// @audit 3 392: if and(gt(itemType, 3), iszero(identifier)) { /// @audit 3 /// @audit 4 394: itemType := sub(3, eq(itemType, 4)) /// @audit 4 683: eq(orderType, 4), /// @audit 1 902: iszero(and(orderType, 1))
File: contracts/lib/SignatureVerification.sol /// @audit 1 73: if iszero(gt(lenDiff, 1)) { /// @audit 1 229: if gt(sub(ECDSA_MaxLength, signatureLength), 1) {
File: contracts/lib/TokenTransferrer.sol /// @audit 1 /// @audit 31 75: and(eq(mload(0), 1), gt(returndatasize(), 31)), /// @audit 1 370: mstore(TokenTransferGenericFailure_error_amount_ptr, 1)
File: contracts/lib/TypehashDirectory.sol /// @audit 1 86: add(shl(OneWordShift, MaxTreeHeight), 1)
File: contracts/lib/Verifiers.sol /// @audit 1 165: let signatureLength := sub(ECDSA_MaxLength, and(fullLength, 1)) /// @audit 1 184: let scratchPtr1 := shl(OneWordShift, and(key, 1)) /// @audit 1 195: let scratchPtr := shl(OneWordShift, and(shr(i, key), 1))
File: contracts/lib/ZoneInteraction.sol /// @audit 2 /// @audit 3 146: or(eq(orderType, 2), eq(orderType, 3)),
File: contracts/Seaport.sol /// @audit 0x20 /// @audit 0x20 104: mstore(0x20, 0x20) /// @audit 0x47 /// @audit 0x07536561706f7274 105: mstore(0x47, 0x07536561706f7274) /// @audit 0x20 /// @audit 0x60 106: return(0x20, 0x60)
This should especially be done if the new value is not required to be different from the old value
There is 1 instance of this issue:
File: contracts/conduit/Conduit.sol /// @audit updateChannel() 202: emit ChannelUpdated(channel, isOpen);
There are 2 instances of this issue:
File: contracts/lib/ConsiderationEncoder.sol /// @audit paramaters 345: // Get the memory pointer to the order paramaters struct.
File: contracts/lib/TypehashDirectory.sol /// @audit writter 31: // Declare an array where each type hash will be writter.
There are 10 instances of this issue:
File: contracts/conduit/lib/ConduitConstants.sol
File: contracts/conduit/lib/ConduitEnums.sol
File: contracts/conduit/lib/ConduitStructs.sol
File: contracts/interfaces/AbridgedTokenInterfaces.sol
File: contracts/interfaces/ContractOffererInterface.sol
File: contracts/interfaces/EIP1271Interface.sol
File: contracts/interfaces/IERC721Receiver.sol
File: contracts/interfaces/ZoneInterface.sol
File: contracts/lib/ConsiderationEnums.sol
File: contracts/lib/TokenTransferrerConstants.sol
There are 2 instances of this issue:
File: contracts/interfaces/TransferHelperInterface.sol /// @audit Missing: '@return' 13 * @param conduitKey The key of the conduit performing the bulk transfer. 14 */ 15 function bulkTransfer( 16 TransferHelperItemsWithRecipient[] calldata items, 17 bytes32 conduitKey 18: ) external returns (bytes4);
File: contracts/lib/ConsiderationEncoder.sol /// @audit Missing: '@param dst' 54 /** 55 * @dev Takes a bytes array in memory and copies it to a new location in 56 * memory. 57 * 58 * @param src A memory pointer referencing the bytes array to be copied (and 59 * pointing to the length of the bytes array). 60 * @param src A memory pointer referencing the location in memory to copy 61 * the bytes array to (and pointing to the length of the copied 62 * bytes array). 63 * 64 * @return size The size of the bytes array. 65 */ 66 function _encodeBytes( 67 MemoryPointer src, 68 MemoryPointer dst 69: ) internal view returns (uint256 size) {
Consider changing the variable to be an unnamed one
There are 13 instances of this issue:
File: contracts/lib/Consideration.sol /// @audit isValidated /// @audit isCancelled /// @audit totalFilled /// @audit totalSize 697 function getOrderStatus( 698 bytes32 orderHash 699 ) 700 external 701 view 702 override 703 returns ( 704 bool isValidated, 705 bool isCancelled, 706 uint256 totalFilled, 707: uint256 totalSize /// @audit version /// @audit domainSeparator /// @audit conduitController 735 function information() 736 external 737 view 738 override 739 returns ( 740 string memory version, 741 bytes32 domainSeparator, 742: address conduitController
File: contracts/lib/OrderValidator.sol /// @audit numerator /// @audit denominator 472 function _getGeneratedOrder( 473 OrderParameters memory orderParameters, 474 bytes memory context, 475 bool revertOnInvalid 476 ) 477 internal 478: returns (bytes32 orderHash, uint256 numerator, uint256 denominator) /// @audit isValidated /// @audit isCancelled /// @audit totalFilled /// @audit totalSize 828 function _getOrderStatus( 829 bytes32 orderHash 830 ) 831 internal 832 view 833 returns ( 834 bool isValidated, 835 bool isCancelled, 836 uint256 totalFilled, 837: uint256 totalSize
Assembly blocks are take a lot more time to audit than normal Solidity code, and often have gotchas and side-effects that the Solidity versions of the same code do not. Consider adding more comments explaining what is being done in every step of the assembly code
There are 6 instances of this issue:
File: contracts/helpers/PointerLibraries.sol 215 assembly { 216 let success := staticcall( 217 gas(), 218 IdentityPrecompileAddress, 219 src, 220 size, 221 dst, 222 size 223 ) 224 if or(iszero(success), iszero(returndatasize())) { 225 revert(0, 0) 226 } 227: }
File: contracts/lib/ConsiderationDecoder.sol 563 assembly { 564 let arrLength := and(calldataload(cdPtrLength), OffsetOrLengthMask) 565 566 // Get the current free memory pointer. 567 mPtrLength := mload(FreeMemoryPointerSlot) 568 569 mstore(mPtrLength, arrLength) 570 let mPtrHead := add(mPtrLength, OneWord) 571 let mPtrTail := add(mPtrHead, shl(OneWordShift, arrLength)) 572 let mPtrTailNext := mPtrTail 573 calldatacopy( 574 mPtrTail, 575 add(cdPtrLength, OneWord), 576 shl(FulfillmentComponent_mem_tail_size_shift, arrLength) 577 ) 578 let mPtrHeadNext := mPtrHead 579 for { 580 581 } lt(mPtrHeadNext, mPtrTail) { 582 583 } { 584 mstore(mPtrHeadNext, mPtrTailNext) 585 mPtrHeadNext := add(mPtrHeadNext, OneWord) 586 mPtrTailNext := add( 587 mPtrTailNext, 588 FulfillmentComponent_mem_tail_size 589 ) 590 } 591 592 // Update the free memory pointer. 593 mstore(FreeMemoryPointerSlot, mPtrTailNext) 594: }
File: contracts/lib/Executor.sol 622 assembly { 623 mstore(accumulator, AccumulatorArmed) // "arm" the accumulator. 624 mstore(add(accumulator, Accumulator_conduitKey_ptr), conduitKey) 625 mstore(add(accumulator, Accumulator_selector_ptr), selector) 626 mstore( 627 add(accumulator, Accumulator_array_offset_ptr), 628 Accumulator_array_offset 629 ) 630 mstore(add(accumulator, Accumulator_array_length_ptr), elements) 631: } 644 assembly { 645 let itemPointer := sub( 646 add(accumulator, mul(elements, Conduit_transferItem_size)), 647 Accumulator_itemSizeOffsetDifference 648 ) 649 mstore(itemPointer, itemType) 650 mstore(add(itemPointer, Conduit_transferItem_token_ptr), token) 651 mstore(add(itemPointer, Conduit_transferItem_from_ptr), from) 652 mstore(add(itemPointer, Conduit_transferItem_to_ptr), to) 653 mstore( 654 add(itemPointer, Conduit_transferItem_identifier_ptr), 655 identifier 656 ) 657 mstore(add(itemPointer, Conduit_transferItem_amount_ptr), amount) 658: }
File: contracts/lib/GettersAndDerivers.sol 326 assembly { 327 mstore(information_version_offset, information_version_cd_offset) 328 mstore(information_domainSeparator_offset, domainSeparator) 329 mstore(information_conduitController_offset, conduitController) 330 mstore(information_versionLengthPtr, information_versionWithLength) 331 return(information_version_offset, information_length) 332: }
File: contracts/lib/Verifiers.sol 122 assembly { 123 validLength := and( 124 lt( 125 sub(signatureLength, BulkOrderProof_minSize), 126 BulkOrderProof_rangeSize 127 ), 128 lt( 129 and( 130 add( 131 signatureLength, 132 BulkOrderProof_lengthAdjustmentBeforeMask 133 ), 134 AlmostOneWord 135 ), 136 BulkOrderProof_lengthRangeAfterMask 137 ) 138 ) 139: }
While 100% code coverage does not guarantee that there are no bugs, it often will catch easy-to-find bugs, and will ensure that there are fewer regressions when the code invariably has to be modified. Furthermore, in order to get full coverage, code authors will often have to re-organize their code so that it is more modular, so that each component can be tested separately, which reduces interdependencies between modules and layers, and makes for code that is easier to reason about and audit.
There is 1 instance of this issue:
File: Various Files
According to the Solidity style guide, functions should be laid out in the following order :constructor(), receive(), fallback(), external, public, internal, private, but the cases below do not follow this pattern
There is 1 instance of this issue:
File: contracts/helpers/PointerLibraries.sol /// @audit setFreeMemoryPointer() came earlier 48 function lt( 49 CalldataPointer a, 50 CalldataPointer b 51: ) internal pure returns (bool c) {
I prefix in the contract nameThere are 9 instances of this issue:
File: contracts/interfaces/AmountDerivationErrors.sol 9: interface AmountDerivationErrors {
File: contracts/interfaces/ConsiderationEventsAndErrors.sol 15: interface ConsiderationEventsAndErrors {
File: contracts/interfaces/CriteriaResolutionErrors.sol 12: interface CriteriaResolutionErrors {
File: contracts/interfaces/FulfillmentApplicationErrors.sol 12: interface FulfillmentApplicationErrors {
File: contracts/interfaces/ReentrancyErrors.sol 9: interface ReentrancyErrors {
File: contracts/interfaces/SignatureVerificationErrors.sol 10: interface SignatureVerificationErrors {
File: contracts/interfaces/TokenTransferrerErrors.sol 7: interface TokenTransferrerErrors {
File: contracts/interfaces/TransferHelperErrors.sol 7: interface TransferHelperErrors {
File: contracts/interfaces/ZoneInteractionErrors.sol 9: interface ZoneInteractionErrors {
These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness
| Issue | Instances | |
|---|---|---|
| [Nā01] | Non-library/interface files should use fixed compiler versions, not floating ones | 24 |
| [Nā02] | Event is missing indexed fields | 6 |
Total: 30 instances over 2 issues
There are 24 instances of this issue:
File: contracts/conduit/Conduit.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.13;
File: contracts/lib/AmountDeriver.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/Assertions.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/BasicOrderFulfiller.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/ConsiderationBase.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/ConsiderationDecoder.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/ConsiderationEncoder.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.13;
File: contracts/lib/Consideration.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/CounterManager.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/CriteriaResolution.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/Executor.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/FulfillmentApplier.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/GettersAndDerivers.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/LowLevelHelpers.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/OrderCombiner.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/OrderFulfiller.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/OrderValidator.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/ReentrancyGuard.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/SignatureVerification.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/TokenTransferrer.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.13;
File: contracts/lib/TypehashDirectory.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/Verifiers.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/lib/ZoneInteraction.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
File: contracts/Seaport.sol /// @audit (valid but excluded finding) 2: pragma solidity ^0.8.17;
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 6 instances of this issue:
File: contracts/interfaces/ConduitInterface.sol /// @audit (valid but excluded finding) 46: event ChannelUpdated(address indexed channel, bool open);
File: contracts/interfaces/ConsiderationEventsAndErrors.sol /// @audit (valid but excluded finding) 31 event OrderFulfilled( 32 bytes32 orderHash, 33 address indexed offerer, 34 address indexed zone, 35 address recipient, 36 SpentItem[] offer, 37 ReceivedItem[] consideration 38: ); /// @audit (valid but excluded finding) 47 event OrderCancelled( 48 bytes32 orderHash, 49 address indexed offerer, 50 address indexed zone 51: ); /// @audit (valid but excluded finding) 61: event OrderValidated(bytes32 orderHash, OrderParameters orderParameters); /// @audit (valid but excluded finding) 69: event OrdersMatched(bytes32[] orderHashes); /// @audit (valid but excluded finding) 77: event CounterIncremented(uint256 newCounter, address indexed offerer);
#0 - c4-judge
2023-01-26T03:03:04Z
HickupHH3 marked the issue as grade-a
š Selected for report: Dravee
Also found by: 0xSmartContract, IllIllI, RaymondFam, Rolezn, atharvasama, c3phas, karanctf, saneryee
1206.4957 USDC - $1,206.50
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [Gā01] | Structs can be packed into fewer storage slots | 3 | - |
| [Gā02] | Using storage instead of memory for structs/arrays saves gas | 9 | 37800 |
| [Gā03] | internal functions only called once can be inlined to save gas | 26 | 520 |
| [Gā04] | Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement | 1 | 85 |
| [Gā05] | Optimize names to save gas | 5 | 110 |
| [Gā06] | internal functions not called by the contract should be removed to save deployment gas | 8 | - |
| [Gā07] | ++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too) | 1 | 5 |
| [Gā08] | Functions guaranteed to revert when called by normal users can be marked payable | 3 | 63 |
Total: 56 instances over 8 issues with 38583 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. The table above as well as its gas numbers do not include any of the excluded findings.
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 are 3 instances of this issue:
File: contracts/lib/ConsiderationStructs.sol /// @audit Variable ordering with 10 slots instead of the current 11: /// user-defined[](32):offer, user-defined[](32):consideration, uint256(32):startTime, uint256(32):endTime, bytes32(32):zoneHash, uint256(32):salt, bytes32(32):conduitKey, uint256(32):counter, address(20):offerer, uint8(1):orderType, address(20):zone 26 struct OrderComponents { 27 address offerer; 28 address zone; 29 OfferItem[] offer; 30 ConsiderationItem[] consideration; 31 OrderType orderType; 32 uint256 startTime; 33 uint256 endTime; 34 bytes32 zoneHash; 35 uint256 salt; 36 bytes32 conduitKey; 37 uint256 counter; 38: } /// @audit Variable ordering with 17 slots instead of the current 18: /// uint256(32):considerationIdentifier, uint256(32):considerationAmount, uint256(32):offerIdentifier, uint256(32):offerAmount, uint256(32):startTime, uint256(32):endTime, bytes32(32):zoneHash, uint256(32):salt, bytes32(32):offererConduitKey, bytes32(32):fulfillerConduitKey, uint256(32):totalOriginalAdditionalRecipients, user-defined[](32):additionalRecipients, bytes(32):signature, address(20):considerationToken, uint8(1):basicOrderType, address(20):offerer, address(20):zone, address(20):offerToken 104 struct BasicOrderParameters { 105 // calldata offset 106 address considerationToken; // 0x24 107 uint256 considerationIdentifier; // 0x44 108 uint256 considerationAmount; // 0x64 109 address payable offerer; // 0x84 110 address zone; // 0xa4 111 address offerToken; // 0xc4 112 uint256 offerIdentifier; // 0xe4 113 uint256 offerAmount; // 0x104 114 BasicOrderType basicOrderType; // 0x124 115 uint256 startTime; // 0x144 116 uint256 endTime; // 0x164 117 bytes32 zoneHash; // 0x184 118 uint256 salt; // 0x1a4 119 bytes32 offererConduitKey; // 0x1c4 120 bytes32 fulfillerConduitKey; // 0x1e4 121 uint256 totalOriginalAdditionalRecipients; // 0x204 122 AdditionalRecipient[] additionalRecipients; // 0x224 123 bytes signature; // 0x244 124 // Total length, excluding dynamic array data: 0x264 (580) 125: } /// @audit Variable ordering with 10 slots instead of the current 11: /// user-defined[](32):offer, user-defined[](32):consideration, uint256(32):startTime, uint256(32):endTime, bytes32(32):zoneHash, uint256(32):salt, bytes32(32):conduitKey, uint256(32):totalOriginalConsiderationItems, address(20):offerer, uint8(1):orderType, address(20):zone 143 struct OrderParameters { 144 address offerer; // 0x00 145 address zone; // 0x20 146 OfferItem[] offer; // 0x40 147 ConsiderationItem[] consideration; // 0x60 148 OrderType orderType; // 0x80 149 uint256 startTime; // 0xa0 150 uint256 endTime; // 0xc0 151 bytes32 zoneHash; // 0xe0 152 uint256 salt; // 0x100 153 bytes32 conduitKey; // 0x120 154 uint256 totalOriginalConsiderationItems; // 0x140 155 // offer.length // 0x160 156: }
storage instead of memory for structs/arrays saves gasWhen fetching data from a storage location, assigning the data to a memory variable causes all fields of the struct/array to be read from storage, which incurs a Gcoldsload (2100 gas) for each field of the struct/array. If the fields are read from the new memory variable, they incur an additional MLOAD rather than a cheap stack read. Instead of declearing the variable with the memory keyword, declaring the variable with the storage keyword and caching any fields that need to be re-read in stack variables, will be much cheaper, only incuring the Gcoldsload for the fields actually read. The only time it makes sense to read the whole struct/array into a memory variable, is if the full struct/array is being returned by the function, is being passed to a function that requires memory, or if the array/struct is being read from another memory array/struct
There are 9 instances of this issue:
File: contracts/lib/CriteriaResolution.sol 91: OfferItem[] memory items = orderParameters.offer;
File: contracts/lib/FulfillmentApplier.sol 72: ReceivedItem memory considerationItem = considerationExecution.item;
File: contracts/lib/OrderCombiner.sol 300: OfferItem memory offerItem = offer[j]; 701: ReceivedItem memory item = execution.item; 752: OrderParameters memory parameters = advancedOrder.parameters; 756: OfferItem[] memory offer = parameters.offer; 763: OfferItem memory offerItem = offer[j];
File: contracts/lib/OrderFulfiller.sol 104: OrderParameters memory orderParameters = advancedOrders[0].parameters;
File: contracts/lib/OrderValidator.sol 758: OrderParameters memory orderParameters = order.parameters;
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 26 instances of this issue:
File: contracts/lib/BasicOrderFulfiller.sol 304 function _prepareBasicFulfillmentFromCalldata( 305 BasicOrderParameters calldata parameters, 306 OrderType orderType, 307 ItemType receivedItemType, 308 ItemType additionalRecipientsItemType, 309 address additionalRecipientsToken, 310 ItemType offeredItemType 311: ) internal returns (bytes32 orderHash) { 916 function _transferNativeTokensAndFinalize( 917 uint256 amount, 918: address payable to 1006 function _transferERC20AndFinalize( 1007 address offerer, 1008 BasicOrderParameters calldata parameters, 1009 bool fromOfferer, 1010: bytes memory accumulator
File: contracts/lib/ConsiderationBase.sol 180 function _deriveTypehashes() 181 internal 182 pure 183 returns ( 184 bytes32 nameHash, 185 bytes32 versionHash, 186 bytes32 eip712DomainTypehash, 187 bytes32 offerItemTypehash, 188 bytes32 considerationItemTypehash, 189: bytes32 orderTypehash
File: contracts/lib/ConsiderationDecoder.sol 233 function _decodeOrderParameters( 234 CalldataPointer cdPtr 235: ) internal pure returns (MemoryPointer mPtr) { 252 function _decodeOrder( 253 CalldataPointer cdPtr 254: ) internal pure returns (MemoryPointer mPtr) { 278 function _decodeAdvancedOrder( 279 CalldataPointer cdPtr 280: ) internal pure returns (MemoryPointer mPtr) { 319 function _getEmptyBytesOrArray() 320 internal 321 pure 322: returns (MemoryPointer mPtr) 336 function _decodeOrderAsAdvancedOrder( 337 CalldataPointer cdPtr 338: ) internal pure returns (MemoryPointer mPtr) { 419 function _decodeCriteriaProof( 420 CalldataPointer cdPtrLength 421: ) internal pure returns (MemoryPointer mPtrLength) { 445 function _decodeCriteriaResolver( 446 CalldataPointer cdPtr 447: ) internal pure returns (MemoryPointer mPtr) { 691 function _decodeFulfillment( 692 CalldataPointer cdPtr 693: ) internal pure returns (MemoryPointer mPtr) {
File: contracts/lib/CriteriaResolution.sol 193 function _updateCriteriaItem( 194 OfferItem[] memory offer, 195 uint256 componentIndex, 196: CriteriaResolver memory criteriaResolver 265 function _verifyProof( 266 uint256 leaf, 267 uint256 root, 268: bytes32[] memory proof
File: contracts/lib/Executor.sol 479: function _trigger(bytes32 conduitKey, bytes memory accumulator) internal {
File: contracts/lib/OrderCombiner.sol 537 function _executeAvailableFulfillments( 538 AdvancedOrder[] memory advancedOrders, 539 FulfillmentComponent[][] memory offerFulfillments, 540 FulfillmentComponent[][] memory considerationFulfillments, 541 bytes32 fulfillerConduitKey, 542 address recipient, 543 bytes32[] memory orderHashes 544 ) 545 internal 546: returns (bool[] memory availableOrders, Execution[] memory executions) 867: function _emitOrdersMatched(bytes32[] memory orderHashes) internal { 981 function _fulfillAdvancedOrders( 982 AdvancedOrder[] memory advancedOrders, 983 Fulfillment[] memory fulfillments, 984 bytes32[] memory orderHashes, 985 address recipient 986: ) internal returns (Execution[] memory executions) {
File: contracts/lib/OrderFulfiller.sol 158 function _applyFractionsAndTransferEach( 159 OrderParameters memory orderParameters, 160 uint256 numerator, 161 uint256 denominator, 162 bytes32 fulfillerConduitKey, 163: address recipient
File: contracts/lib/OrderValidator.sol 439 function _checkRecipients( 440 address originalRecipient, 441 address newRecipient 442: ) internal pure returns (uint256 isInvalid) { 472 function _getGeneratedOrder( 473 OrderParameters memory orderParameters, 474 bytes memory context, 475 bool revertOnInvalid 476 ) 477 internal 478: returns (bytes32 orderHash, uint256 numerator, uint256 denominator) 890 function _doesNotSupportPartialFills( 891 OrderType orderType, 892 uint256 numerator, 893 uint256 denominator 894: ) internal pure returns (bool isFullOrder) {
File: contracts/lib/TypehashDirectory.sol 99 function getMaxTreeBrackets( 100 uint256 maxHeight 101: ) internal pure returns (bytes memory) { 131: function getTreeSubTypes() internal pure returns (bytes memory) {
File: contracts/lib/Verifiers.sol 117 function _isValidBulkOrderSize( 118 uint256 signatureLength 119: ) internal pure returns (bool validLength) { 150 function _computeBulkOrderProof( 151 bytes memory proofAndSignature, 152 bytes32 leaf 153: ) internal view returns (bytes32 bulkOrderHash) {
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/lib/BasicOrderFulfiller.sol /// @audit if-condition on line 981 987: nativeTokensRemaining - amount
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 5 instances of this issue:
File: contracts/interfaces/ConduitInterface.sol /// @audit execute(), executeBatch1155(), executeWithBatch1155(), updateChannel() 15: interface ConduitInterface {
File: contracts/interfaces/ConsiderationInterface.sol /// @audit fulfillBasicOrder(), fulfillOrder(), fulfillAdvancedOrder(), fulfillAvailableOrders(), fulfillAvailableAdvancedOrders(), matchOrders(), matchAdvancedOrders(), cancel(), validate(), incrementCounter(), getOrderHash(), getOrderStatus(), getCounter(), information(), getContractOffererNonce() 27: interface ConsiderationInterface {
File: contracts/interfaces/ContractOffererInterface.sol /// @audit generateOrder(), ratifyOrder(), previewOrder(), getMetadata() 6: interface ContractOffererInterface {
File: contracts/interfaces/ImmutableCreate2FactoryInterface.sol /// @audit safeCreate2(), findCreate2Address(), findCreate2AddressViaHash(), hasBeenDeployed() 18: interface ImmutableCreate2FactoryInterface {
File: contracts/interfaces/SeaportInterface.sol /// @audit fulfillBasicOrder(), fulfillOrder(), fulfillAdvancedOrder(), fulfillAvailableOrders(), fulfillAvailableAdvancedOrders(), matchOrders(), matchAdvancedOrders(), cancel(), validate(), incrementCounter(), getOrderHash(), getOrderStatus(), getCounter(), information(), getContractOffererNonce() 26: interface SeaportInterface {
internal functions not called by the contract should be removed to save deployment gasIf the functions are required by an interface, the contract should inherit from that interface and use the override keyword
There are 8 instances of this issue:
File: contracts/lib/ConsiderationDecoder.sol 378 function _decodeOrdersAsAdvancedOrders( 379 CalldataPointer cdPtrLength 380: ) internal pure returns (MemoryPointer mPtrLength) { 477 function _decodeCriteriaResolvers( 478 CalldataPointer cdPtrLength 479: ) internal pure returns (MemoryPointer mPtrLength) { 517 function _decodeOrders( 518 CalldataPointer cdPtrLength 519: ) internal pure returns (MemoryPointer mPtrLength) { 609 function _decodeNestedFulfillmentComponents( 610 CalldataPointer cdPtrLength 611: ) internal pure returns (MemoryPointer mPtrLength) { 652 function _decodeAdvancedOrders( 653 CalldataPointer cdPtrLength 654: ) internal pure returns (MemoryPointer mPtrLength) { 723 function _decodeFulfillments( 724 CalldataPointer cdPtrLength 725: ) internal pure returns (MemoryPointer mPtrLength) { 764 function _decodeOrderComponentsAsOrderParameters( 765 CalldataPointer cdPtr 766: ) internal pure returns (MemoryPointer mPtr) { 804 function _decodeGenerateOrderReturndata() 805 internal 806 pure 807 returns ( 808 uint256 invalidEncoding, 809 MemoryPointer offer, 810: MemoryPointer consideration
++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)Saves 5 gas per loop
There is 1 instance of this issue:
File: contracts/lib/OrderCombiner.sol 272: maximumFulfilled--;
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 3 instances of this issue:
File: contracts/conduit/Conduit.sol 92 function execute( 93 ConduitTransfer[] calldata transfers 94: ) external override onlyOpenChannel returns (bytes4 magicValue) { 127 function executeBatch1155( 128 ConduitBatch1155Transfer[] calldata batchTransfers 129: ) external override onlyOpenChannel returns (bytes4 magicValue) { 154 function executeWithBatch1155( 155 ConduitTransfer[] calldata standardTransfers, 156 ConduitBatch1155Transfer[] calldata batchTransfers 157: ) external override onlyOpenChannel returns (bytes4 magicValue) {
These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [Gā01] | Using bools for storage incurs overhead | 1 | 17100 |
Total: 1 instances over 1 issues with 17100 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. The table above as well as its gas numbers do not include any of the excluded findings.
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 is 1 instance of this issue:
File: contracts/conduit/Conduit.sol /// @audit (valid but excluded finding) 34: mapping(address => bool) private _channels;
#0 - c4-judge
2023-01-26T03:29:56Z
HickupHH3 marked the issue as grade-a
#1 - c4-judge
2023-01-26T04:10:58Z
HickupHH3 marked the issue as grade-b
#2 - HickupHH3
2023-01-26T04:13:51Z
Downgraded because of failure to filter false positives. Examples:
[Gā04] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement
Already unchecked.
if (nativeTokensRemaining > amount) { // Skip underflow check as nativeTokensRemaining > amount. unchecked { // Transfer remaining native tokens to the caller. _transferNativeTokens( payable(msg.sender), nativeTokensRemaining - amount ); } }
[Gā06] internal functions not called by the contract should be removed to save deployment gas
Referenced functions are called in higher order function
#3 - c4-judge
2023-01-26T06:38:01Z
HickupHH3 marked the issue as grade-a
#4 - HickupHH3
2023-01-26T06:38:14Z
Moderation applied, bumped back up to grade A