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
Rank: 9/62
Findings: 2
Award: $920.83
š Selected for report: 0
š Solo Findings: 0
š Selected for report: 0xSmartContract
Also found by: 0x4non, 0xNazgul, 0xhacksmithh, Aymen0909, HE1M, IllIllI, Josiah, RaymondFam, Rolezn, Tricko, brgltd, c3phas, chrisdior4, joestakey, ladboy233, martin, neko_nyaa, rotcivegaf, tnevler, trustindistrust
529.4504 USDC - $529.45
Issue | Instances | |
---|---|---|
[Lā01] | Upgradeable contract not initialized | 3 |
[Lā02] | Open TODOs | 1 |
[Lā03] | Vulnerable to cross-chain replay attacks due to DOMAIN_SEPARATOR never changing | 1 |
Total: 5 instances over 3 issues
Issue | Instances | |
---|---|---|
[Nā01] | Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions | 2 |
[Nā02] | require() /revert() statements should have descriptive reason strings | 7 |
[Nā03] | public functions not called by the contract should be declared external instead | 4 |
[Nā04] | constant s should be defined rather than using magic numbers | 16 |
[Nā05] | Lines are too long | 1 |
[Nā06] | NatSpec is incomplete | 9 |
[Nā07] | Event is missing indexed fields | 5 |
Total: 44 instances over 7 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 are 3 instances of this issue:
File: contracts/Exchange.sol /// @audit missing __UUPS_init() 30: contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, UUPSUpgradeable {
File: contracts/Pool.sol /// @audit missing __Ownable_init() /// @audit missing __UUPS_init() 13: contract Pool is IPool, OwnableUpgradeable, UUPSUpgradeable {
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
DOMAIN_SEPARATOR
never changingSee 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;
__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 are 2 instances of this issue:
File: contracts/Exchange.sol 30: contract Exchange is IExchange, ReentrancyGuarded, EIP712, OwnableUpgradeable, UUPSUpgradeable {
File: contracts/Pool.sol 13: contract Pool is IPool, OwnableUpgradeable, UUPSUpgradeable {
require()
/revert()
statements should have descriptive reason stringsThere 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);
File: contracts/Pool.sol 45: require(_balances[msg.sender] >= amount); 48: require(success); 71: require(_balances[from] >= amount); 72: require(to != address(0));
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 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) {
constant
s should be defined rather than using magic numbersEven 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");
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.
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
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)
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 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);
File: contracts/Pool.sol 23: event Transfer(address indexed from, address indexed to, uint256 amount);
#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
š Selected for report: ReyAdmirado
Also found by: 0x4non, 0xRoxas, 0xab00, Awesome, Aymen0909, Bnke0x0, Deivitto, Diana, IllIllI, Rahoz, RaymondFam, Rolezn, Sathish9098, ajtra, aphak5010, aviggiano, c3phas, carlitox477, ch0bu, cryptostellar5, erictee, lukris02, martin, rotcivegaf, saian, shark, trustindistrust, zaskoh
391.3828 USDC - $391.38
Issue | Instances | Total Gas Saved | |
---|---|---|---|
[Gā01] | State variables can be packed into fewer storage slots | 1 | - |
[Gā02] | <x> += <y> costs more gas than <x> = <x> + <y> for state variables | 1 | 113 |
[Gā03] | internal functions only called once can be inlined to save gas | 6 | 120 |
[Gā04] | Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if -statement | 1 | 85 |
[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 | 3 | 180 |
[Gā06] | require() /revert() strings longer than 32 bytes cost extra gas | 4 | - |
[Gā07] | Optimize names to save gas | 1 | 22 |
[Gā08] | Functions guaranteed to revert when called by normal users can be marked payable | 8 | 168 |
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.
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;
<x> += <y>
costs more gas than <x> = <x> + <y>
for state variablesUsing the addition operator instead of plus-equals saves 113 gas
There is 1 instance of this issue:
File: contracts/Exchange.sol 574: remainingETH -= price;
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 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
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/Exchange.sol /// @audit require() on line 604 607: uint256 receiveAmount = price - totalFee;
++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 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++) {
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 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");
File: contracts/Pool.sol 63: revert('Caller is not authorized to transfer');
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 {
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
File: contracts/Pool.sol 15: function _authorizeUpgrade(address) internal override onlyOwner {}
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] | <array>.length should not be looked up in every loop of a for -loop | 2 | 6 |
[Gā02] | Using bool s for storage incurs overhead | 2 | 34200 |
[Gā03] | ++i costs less gas than i++ , especially when it's used in for -loops (--i /i-- too) | 3 | 15 |
[Gā04] | Using private rather than public for constants, saves gas | 3 | - |
[Gā05] | Use custom errors rather than revert() /require() strings to save gas | 19 | - |
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.
<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 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++) {
bool
s 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;
++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++) {
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 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;
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 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");
#0 - c4-judge
2022-11-17T14:39:51Z
berndartmueller marked the issue as grade-a