Blur Exchange contest - IllIllI's results

An NFT exchange.

General Information

Platform: Code4rena

Start Date: 11/11/2022

Pot Size: $36,500 USDC

Total HM: 5

Participants: 62

Period: 3 days

Judge: berndartmueller

Id: 181

League: ETH

Blur Exchange

Findings Distribution

Researcher Performance

Rank: 9/62

Findings: 2

Award: $920.83

QA:
grade-a
Gas:
grade-a

🌟 Selected for report: 0

šŸš€ Solo Findings: 0

Awards

529.4504 USDC - $529.45

Labels

bug
grade-a
QA (Quality Assurance)
edited-by-warden
Q-01

External Links

Summary

Low Risk Issues

IssueInstances
[L‑01]Upgradeable contract not initialized3
[L‑02]Open TODOs1
[L‑03]Vulnerable to cross-chain replay attacks due to DOMAIN_SEPARATOR never changing1

Total: 5 instances over 3 issues

Non-critical Issues

IssueInstances
[N‑01]Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions2
[N‑02]require()/revert() statements should have descriptive reason strings7
[N‑03]public functions not called by the contract should be declared external instead4
[N‑04]constants should be defined rather than using magic numbers16
[N‑05]Lines are too long1
[N‑06]NatSpec is incomplete9
[N‑07]Event is missing indexed fields5

Total: 44 instances over 7 issues

Low Risk Issues

[L‑01] Upgradeable contract not initialized

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 are 3 instances of this issue:

File: contracts/Exchange.sol

/// @audit missing __UUPS_init()
30:   contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, UUPSUpgradeable {

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L30

File: contracts/Pool.sol

/// @audit missing __Ownable_init()
/// @audit missing __UUPS_init()
13:   contract Pool is IPool, OwnableUpgradeable, UUPSUpgradeable {

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L13

[L‑02] Open TODOs

Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment

There is 1 instance of this issue:

File: contracts/Pool.sol

18:       // TODO: set proper address before deployment

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L18

[L‑03] Vulnerable to cross-chain replay attacks due to DOMAIN_SEPARATOR never changing

See this issue from a prior contest for details

There is 1 instance of this issue:

// File: contracts/Exchange.sol

112        function initialize(
113            IExecutionDelegate _executionDelegate,
114            IPolicyManager _policyManager,
115            address _oracle,
116            uint _blockRange
117        ) external initializer {
118            __Ownable_init();
119            isOpen = 1;
120    
121 @>         DOMAIN_SEPARATOR = _hashDomain(EIP712Domain({
122                name              : NAME,
123                version           : VERSION,
124                chainId           : block.chainid,
125                verifyingContract : address(this)
126            }));
127    
128            executionDelegate = _executionDelegate;
129            policyManager = _policyManager;
130            oracle = _oracle;
131:           blockRange = _blockRange;

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L112-L131

Non-critical Issues

[N‑01] Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions

See 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 are 2 instances of this issue:

File: contracts/Exchange.sol

30:   contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, UUPSUpgradeable {

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L30

File: contracts/Pool.sol

13:   contract Pool is IPool, OwnableUpgradeable, UUPSUpgradeable {

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L13

[N‑02] require()/revert() statements should have descriptive reason strings

There are 7 instances of this issue:

File: contracts/Exchange.sol

240:          require(sell.order.side == Side.Sell);

291:          require(msg.sender == order.trader);

573:              require(remainingETH >= price);

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L240

File: contracts/Pool.sol

45:           require(_balances[msg.sender] >= amount);

48:           require(success);

71:           require(_balances[from] >= amount);

72:           require(to != address(0));

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L45

[N‑03] public functions not called by the contract should be declared external instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

There are 4 instances of this issue:

File: contracts/Pool.sol

44:       function withdraw(uint256 amount) public {

58        function transferFrom(address from, address to, uint256 amount)
59            public
60:           returns (bool)

79:       function balanceOf(address user) public view returns (uint256) {

83:       function totalSupply() public view returns (uint256) {

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L44

[N‑04] constants should be defined rather than using magic numbers

Even assembly can benefit from using readable constants instead of hex/numeric literals

There are 16 instances of this issue:

File: contracts/Exchange.sol

/// @audit 0x40
186:                  let memPointer := mload(0x40)

/// @audit 0x20
188:                  let order_location := calldataload(add(executions.offset, mul(i, 0x20)))

/// @audit 0x01
192:                  switch eq(add(i, 0x01), executionsLength)

/// @audit 0x01
/// @audit 0x20
197:                      let next_order_location := calldataload(add(executions.offset, mul(add(i, 0x01), 0x20)))

/// @audit 0xe04d94ae00000000000000000000000000000000000000000000000000000000
202:                  mstore(memPointer, 0xe04d94ae00000000000000000000000000000000000000000000000000000000) // _execute

/// @audit 0x04
203:                  calldatacopy(add(0x04, memPointer), order_pointer, size)

/// @audit 0x04
206:                  let result := delegatecall(gas(), address(), memPointer, add(size, 0x04), 0, 0)

/// @audit 0x01
412:          if (order.order.extraParams.length > 0 && order.order.extraParams[0] == 0x01) {

/// @audit 0x20
483:                  r := calldataload(add(extraSignature.offset, 0x20))

/// @audit 0x40
484:                  s := calldataload(add(extraSignature.offset, 0x40))

/// @audit 0x20
493:                  v := calldataload(add(extraSignature.offset, 0x20))

/// @audit 0x40
494:                  r := calldataload(add(extraSignature.offset, 0x40))

/// @audit 0x60
495:                  s := calldataload(add(extraSignature.offset, 0x60))

/// @audit 27
/// @audit 28
523:          require(v == 27 || v == 28, "Invalid v parameter");

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L186

[N‑05] Lines are too long

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 is 1 instance of this issue:

File: contracts/Exchange.sol

230:       * @dev Match two orders, ensuring validity of the match, and execute all associated state transitions. Protected against reentrancy by a contract-global lock. Must be called internally.

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L230

[N‑06] NatSpec is incomplete

There are 9 instances of this issue:

File: contracts/Exchange.sol

/// @audit Missing: '@return'
364        * @param orderHash hash of order
365        */
366       function _validateOrderParameters(Order calldata order, bytes32 orderHash)
367           internal
368           view
369:          returns (bool)

/// @audit Missing: '@return'
385        * @param orderHash hash of order
386        */
387       function _validateSignatures(Input calldata order, bytes32 orderHash)
388           internal
389           view
390:          returns (bool)

/// @audit Missing: '@return'
438        * @param extraSignature packed merkle path
439        */
440       function _validateUserAuthorization(
441           bytes32 orderHash,
442           address trader,
443           uint8 v,
444           bytes32 r,
445           bytes32 s,
446           SignatureVersion signatureVersion,
447           bytes calldata extraSignature
448:      ) internal view returns (bool) {

/// @audit Missing: '@return'
469        * @param blockNumber block number used in oracle signature
470        */
471       function _validateOracleAuthorization(
472           bytes32 orderHash,
473           SignatureVersion signatureVersion,
474           bytes calldata extraSignature,
475           uint256 blockNumber
476:      ) internal view returns (bool) {

/// @audit Missing: '@return'
514        * @param s s
515        */
516       function _verify(
517           address signer,
518           bytes32 digest,
519           uint8 v,
520           bytes32 r,
521           bytes32 s
522:      ) internal pure returns (bool) {

/// @audit Missing: '@return'
535        * @param buy buy order
536        */
537       function _canMatchOrders(Order calldata sell, Order calldata buy)
538           internal
539           view
540:          returns (uint256 price, uint256 tokenId, uint256 amount, AssetType assetType)

/// @audit Missing: '@return'
589        * @param price price of token
590        */
591       function _transferFees(
592           Fee[] calldata fees,
593           address paymentToken,
594           address from,
595           uint256 price
596:      ) internal returns (uint256) {

/// @audit Missing: '@param amount'
645       /**
646        * @dev Execute call through delegate proxy
647        * @param collection collection contract address
648        * @param from seller address
649        * @param to buyer address
650        * @param tokenId tokenId
651        * @param assetType asset type of the token
652        */
653       function _executeTokenTransfer(
654           address collection,
655           address from,
656           address to,
657           uint256 tokenId,
658           uint256 amount,
659:          AssetType assetType

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L364-L369

File: contracts/Pool.sol

/// @audit Missing: '@return'
56         * @param amount Amount to transfer
57         */
58        function transferFrom(address from, address to, uint256 amount)
59            public
60:           returns (bool)

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L56-L60

[N‑07] Event is missing indexed fields

Index 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 5 instances of this issue:

File: contracts/Exchange.sol

90        event OrdersMatched(
91            address indexed maker,
92            address indexed taker,
93            Order sell,
94            bytes32 sellHash,
95            Order buy,
96            bytes32 buyHash
97:       );

99:       event OrderCancelled(bytes32 hash);

100:      event NonceIncremented(address indexed trader, uint256 newNonce);

105:      event NewBlockRange(uint256 blockRange);

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L90-L97

File: contracts/Pool.sol

23:       event Transfer(address indexed from, address indexed to, uint256 amount);

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L23

#0 - c4-judge

2022-11-15T21:36:16Z

berndartmueller marked the issue as grade-b

#1 - c4-judge

2022-11-17T12:49:47Z

berndartmueller marked the issue as grade-a

Awards

391.3828 USDC - $391.38

Labels

bug
G (Gas Optimization)
grade-a
G-04

External Links

Summary

Gas Optimizations

IssueInstancesTotal Gas Saved
[G‑01]State variables can be packed into fewer storage slots1-
[G‑02]<x> += <y> costs more gas than <x> = <x> + <y> for state variables1113
[G‑03]internal functions only called once can be inlined to save gas6120
[G‑04]Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement185
[G‑05]++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-loops3180
[G‑06]require()/revert() strings longer than 32 bytes cost extra gas4-
[G‑07]Optimize names to save gas122
[G‑08]Functions guaranteed to revert when called by normal users can be marked payable8168

Total: 25 instances over 8 issues with 688 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.

Gas Optimizations

[G‑01] State variables can be packed into fewer storage slots

If variables occupying the same slot are both written the same function or by the constructor, avoids a separate Gsset (20000 gas). Reads of the variables can also be cheaper

There is 1 instance of this issue:

File: contracts/Exchange.sol

/// @audit Variable ordering with 8 slots instead of the current 9:
///           uint256(32):isOpen, uint256(32):blockRange, mapping(32):cancelledOrFilled, mapping(32):nonces, uint256(32):remainingETH, user-defined(20):executionDelegate, bool(1):isInternal, user-defined(20):policyManager, address(20):oracle
33:       uint256 public isOpen;

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L33

[G‑02] <x> += <y> costs more gas than <x> = <x> + <y> for state variables

Using the addition operator instead of plus-equals saves 113 gas

There is 1 instance of this issue:

File: contracts/Exchange.sol

574:              remainingETH -= price;

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L574

[G‑03] internal functions only called once can be inlined to save gas

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

There are 6 instances of this issue:

File: contracts/Exchange.sol

440       function _validateUserAuthorization(
441           bytes32 orderHash,
442           address trader,
443           uint8 v,
444           bytes32 r,
445           bytes32 s,
446           SignatureVersion signatureVersion,
447           bytes calldata extraSignature
448:      ) internal view returns (bool) {

471       function _validateOracleAuthorization(
472           bytes32 orderHash,
473           SignatureVersion signatureVersion,
474           bytes calldata extraSignature,
475           uint256 blockNumber
476:      ) internal view returns (bool) {

537       function _canMatchOrders(Order calldata sell, Order calldata buy)
538           internal
539           view
540:          returns (uint256 price, uint256 tokenId, uint256 amount, AssetType assetType)

565       function _executeFundsTransfer(
566           address seller,
567           address buyer,
568           address paymentToken,
569           Fee[] calldata fees,
570:          uint256 price

591       function _transferFees(
592           Fee[] calldata fees,
593           address paymentToken,
594           address from,
595           uint256 price
596:      ) internal returns (uint256) {

653       function _executeTokenTransfer(
654           address collection,
655           address from,
656           address to,
657           uint256 tokenId,
658           uint256 amount,
659:          AssetType assetType

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L440-L448

[G‑04] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement

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

There is 1 instance of this issue:

File: contracts/Exchange.sol

/// @audit require() on line 604
607:          uint256 receiveAmount = price - totalFee;

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L607

[G‑05] ++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

The 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 3 instances of this issue:

File: contracts/Exchange.sol

184:          for (uint8 i = 0; i < executionsLength; i++) {

307:          for (uint8 i = 0; i < orders.length; i++) {

598:          for (uint8 i = 0; i < fees.length; i++) {

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L184

[G‑06] require()/revert() strings longer than 32 bytes cost extra gas

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

There are 4 instances of this issue:

File: contracts/Exchange.sol

49:           require(isInternal, "This function should not be called directly");

295:          require(!cancelledOrFilled[hash], "Order already cancelled or filled");

604:          require(totalFee <= price, "Total amount of fees are more than the price");

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L49

File: contracts/Pool.sol

63:               revert('Caller is not authorized to transfer');

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L63

[G‑07] Optimize names to save gas

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 is 1 instance of this issue:

File: contracts/Exchange.sol

/// @audit open(), close(), initialize(), updateDomainSeparator(), execute(), bulkExecute(), _execute(), cancelOrder(), cancelOrders(), incrementNonce(), setExecutionDelegate(), setPolicyManager(), setOracle(), setBlockRange()
30:   contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, UUPSUpgradeable {

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L30

[G‑08] Functions guaranteed to revert when called by normal users can be marked payable

If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost

There are 8 instances of this issue:

File: contracts/Exchange.sol

56:       function open() external onlyOwner {

60:       function close() external onlyOwner {

66:       function _authorizeUpgrade(address) internal override onlyOwner {}

323       function setExecutionDelegate(IExecutionDelegate _executionDelegate)
324           external
325:          onlyOwner

332       function setPolicyManager(IPolicyManager _policyManager)
333           external
334:          onlyOwner

341       function setOracle(address _oracle)
342           external
343:          onlyOwner

350       function setBlockRange(uint256 _blockRange)
351           external
352:          onlyOwner

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L56

File: contracts/Pool.sol

15:       function _authorizeUpgrade(address) internal override onlyOwner {}

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Pool.sol#L15


Excluded findings

These findings are excluded from awards calculations because there are publicly-available automated tools that find them. The valid ones appear here for completeness

Summary

Gas Optimizations

IssueInstancesTotal Gas Saved
[G‑01]<array>.length should not be looked up in every loop of a for-loop26
[G‑02]Using bools for storage incurs overhead234200
[G‑03]++i costs less gas than i++, especially when it's used in for-loops (--i/i-- too)315
[G‑04]Using private rather than public for constants, saves gas3-
[G‑05]Use custom errors rather than revert()/require() strings to save gas19-

Total: 29 instances over 5 issues with 34221 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.

Gas Optimizations

[G‑01] <array>.length should not be looked up in every loop of a for-loop

The overheads outlined below are PER LOOP, excluding the first loop

  • storage arrays incur a Gwarmaccess (100 gas)
  • memory arrays use MLOAD (3 gas)
  • calldata arrays use 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 2 instances of this issue:

File: contracts/Exchange.sol

/// @audit (valid but excluded finding)
307:          for (uint8 i = 0; i < orders.length; i++) {

/// @audit (valid but excluded finding)
598:          for (uint8 i = 0; i < fees.length; i++) {

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L307

[G‑02] Using 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 2 instances of this issue:

File: contracts/Exchange.sol

/// @audit (valid but excluded finding)
85:       mapping(bytes32 => bool) public cancelledOrFilled;

/// @audit (valid but excluded finding)
146:      bool public isInternal = false;

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L85

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

Saves 5 gas per loop

There are 3 instances of this issue:

File: contracts/Exchange.sol

/// @audit (valid but excluded finding)
184:          for (uint8 i = 0; i < executionsLength; i++) {

/// @audit (valid but excluded finding)
307:          for (uint8 i = 0; i < orders.length; i++) {

/// @audit (valid but excluded finding)
598:          for (uint8 i = 0; i < fees.length; i++) {

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L184

[G‑04] Using private rather than public for constants, saves gas

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

There are 3 instances of this issue:

File: contracts/Exchange.sol

/// @audit (valid but excluded finding)
70:       string public constant NAME = "Exchange";

/// @audit (valid but excluded finding)
71:       string public constant VERSION = "1.0";

/// @audit (valid but excluded finding)
72:       uint256 public constant INVERSE_BASIS_POINT = 10_000;

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L70

[G‑05] Use custom errors rather than revert()/require() strings to save gas

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

There are 19 instances of this issue:

File: contracts/Exchange.sol

/// @audit (valid but excluded finding)
36:           require(isOpen == 1, "Closed");

/// @audit (valid but excluded finding)
49:           require(isInternal, "This function should not be called directly");

/// @audit (valid but excluded finding)
245:          require(_validateSignatures(sell, sellHash), "Sell failed authorization");

/// @audit (valid but excluded finding)
246:          require(_validateSignatures(buy, buyHash), "Buy failed authorization");

/// @audit (valid but excluded finding)
248:          require(_validateOrderParameters(sell.order, sellHash), "Sell has invalid parameters");

/// @audit (valid but excluded finding)
249:          require(_validateOrderParameters(buy.order, buyHash), "Buy has invalid parameters");

/// @audit (valid but excluded finding)
295:          require(!cancelledOrFilled[hash], "Order already cancelled or filled");

/// @audit (valid but excluded finding)
327:          require(address(_executionDelegate) != address(0), "Address cannot be zero");

/// @audit (valid but excluded finding)
336:          require(address(_policyManager) != address(0), "Address cannot be zero");

/// @audit (valid but excluded finding)
345:          require(_oracle != address(0), "Address cannot be zero");

/// @audit (valid but excluded finding)
414:              require(block.number - order.blockNumber < blockRange, "Signed block number out of range");

/// @audit (valid but excluded finding)
523:          require(v == 27 || v == 28, "Invalid v parameter");

/// @audit (valid but excluded finding)
545:              require(policyManager.isPolicyWhitelisted(sell.matchingPolicy), "Policy is not whitelisted");

/// @audit (valid but excluded finding)
549:              require(policyManager.isPolicyWhitelisted(buy.matchingPolicy), "Policy is not whitelisted");

/// @audit (valid but excluded finding)
552:          require(canMatch, "Orders cannot be matched");

/// @audit (valid but excluded finding)
604:          require(totalFee <= price, "Total amount of fees are more than the price");

/// @audit (valid but excluded finding)
630:              require(to != address(0), "Transfer to zero address");

/// @audit (valid but excluded finding)
632:              require(success, "ETH transfer failed");

/// @audit (valid but excluded finding)
636:              require(success, "Pool transfer failed");

https://github.com/code-423n4/2022-11-non-fungible/blob/323b7cbf607425dd81da96c0777c8b12e800305d/contracts/Exchange.sol#L36

#0 - c4-judge

2022-11-17T14:39:51Z

berndartmueller marked the issue as grade-a

AuditHub

A portfolio for auditors, a security profile for protocols, a hub for web3 security.

Built bymalatrax Ā© 2024

Auditors

Browse

Contests

Browse

Get in touch

ContactTwitter