Platform: Code4rena
Start Date: 07/10/2022
Pot Size: $50,000 USDC
Total HM: 4
Participants: 62
Period: 5 days
Judge: 0xean
Total Solo HM: 2
Id: 169
League: ETH
Rank: 54/62
Findings: 1
Award: $20.79
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: IllIllI
Also found by: 0x1f8b, 0xNazgul, 0xSmartContract, 0xdeadbeef, B2, Bnke0x0, Deivitto, ElKu, Jujic, KoKo, Pheonix, RaymondFam, RedOneN, RockingMiles, Rolezn, Saintcode_, Shinchan, TomJ, Tomio, __141345__, ajtra, aysha, c3phas, carlitox477, catchup, delfin454000, emrekocak, erictee, fatherOfBlocks, gerdusx, gianganhnguyen, gogo, martin, mcwildy, medikko, oyc_109, pedr02b2, rbserver, ret2basic, rotcivegaf, saian, sakman, zishansami
20.7905 USDC - $20.79
| Serial No. | Issue Name | Instances |
|---|---|---|
| G-1 | Using > 0 costs more gas than != 0 when used on a uint in a require() statement | 3 |
| G-2 | Splitting require() statements that use && saves gas | 1 |
| G-3 | Use custom errors rather than revert()/require() strings to save deployment gas | 55 |
| G-4 | Usage of assert() instead of require() | 3 |
| G-5 | Expressions for constant values such as a call to keccak256(), should use immutable rather than constant | 2 |
| G-6 | abi.encode() is less efficient than abi.encodePacked() | 6 |
| G-7 | Reduce the size of error messages (Long revert Strings) | 4 |
| G-8 | Use calldata instead of memory in external functions | 2 |
| G-9 | Using bools for storage incurs overhead | 4 |
| G-10 | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 8 |
| G-11 | Use a more recent version of solidity | 23 |
| G-12 | Use Assembly to check for address(0) | 23 |
| G-13 | Inline a modifier that’s only used once | 5 |
Total instances found in this contest: 139 | Among all files in scope
0 is less efficient than != 0 for unsigned integers (with proof) != 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas) Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you’re in a require statement, this will save gas. You can see this tweet for more proof: https://twitter.com/gzeon/status/1485428085885640706
contracts/l2/gateway/L2GraphTokenGateway.sol:146: require(_amount > 0, "INVALID_ZERO_AMOUNT");
contracts/gateway/L1GraphTokenGateway.sol:201: require(_amount > 0, "INVALID_ZERO_AMOUNT");
contracts/gateway/L1GraphTokenGateway.sol:217: require(maxSubmissionCost > 0, "NO_SUBMISSION_COST");
https://twitter.com/gzeon/status/1485428085885640706
I suggest changing > 0 with != 0. Also, please enable the Optimizer.
Require statements including conditions with the && operator can be broken down in multiple require statements to save gas.
contracts/gateway/L1GraphTokenGateway.sol:142: require(_escrow != address(0) && Address.isContract(_escrow), "INVALID_ESCROW");
Breakdown each condition in a separate require statement (though require statements should be replaced with custom errors)
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met). Starting from Solidity v0.8.4,there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");),but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them. Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).
contracts/l2/gateway/L2GraphTokenGateway.sol:98: require(_l2Router != address(0), "INVALID_L2_ROUTER");
contracts/l2/gateway/L2GraphTokenGateway.sol:108: require(_l1GRT != address(0), "INVALID_L1_GRT");
contracts/l2/gateway/L2GraphTokenGateway.sol:118: require(_l1Counterpart != address(0), "INVALID_L1_COUNTERPART");
contracts/l2/gateway/L2GraphTokenGateway.sol:145: require(_l1Token == l1GRT, "TOKEN_NOT_GRT");
contracts/l2/gateway/L2GraphTokenGateway.sol:146: require(_amount > 0, "INVALID_ZERO_AMOUNT");
contracts/l2/gateway/L2GraphTokenGateway.sol:147: require(msg.value == 0, "INVALID_NONZERO_VALUE");
contracts/l2/gateway/L2GraphTokenGateway.sol:148: require(_to != address(0), "INVALID_DESTINATION");
contracts/l2/gateway/L2GraphTokenGateway.sol:153: require(outboundCalldata.extraData.length == 0, "CALL_HOOK_DATA_NOT_ALLOWED");
contracts/l2/gateway/L2GraphTokenGateway.sol:233: require(_l1Token == l1GRT, "TOKEN_NOT_GRT");
contracts/l2/gateway/L2GraphTokenGateway.sol:234: require(msg.value == 0, "INVALID_NONZERO_VALUE");
contracts/l2/token/L2GraphToken.sol:36: require(msg.sender == gateway, "NOT_GATEWAY");
contracts/l2/token/L2GraphToken.sol:49: require(_owner != address(0), "Owner must be set");
contracts/l2/token/L2GraphToken.sol:60: require(_gw != address(0), "INVALID_GATEWAY");
contracts/l2/token/L2GraphToken.sol:70: require(_addr != address(0), "INVALID_L1_ADDRESS");
contracts/l2/token/GraphTokenUpgradeable.sol:60: require(isMinter(msg.sender), "Only minter can call");
contracts/l2/token/GraphTokenUpgradeable.sol:94: require(_owner == recoveredAddress, "GRT: invalid permit");
contracts/l2/token/GraphTokenUpgradeable.sol:95: require(_deadline == 0 || block.timestamp <= _deadline, "GRT: expired permit");
GraphTokenUpgradeable.sol:L106
contracts/l2/token/GraphTokenUpgradeable.sol:106: require(_account != address(0), "INVALID_MINTER");
GraphTokenUpgradeable.sol:L115
contracts/l2/token/GraphTokenUpgradeable.sol:115: require(_minters[_account], "NOT_A_MINTER");
GraphTokenUpgradeable.sol:L123
contracts/l2/token/GraphTokenUpgradeable.sol:123: require(_minters[msg.sender], "NOT_A_MINTER");
contracts/upgrades/GraphProxy.sol:105: require(_newAdmin != address(0), "Cannot change the admin of a proxy to the zero address");
contracts/upgrades/GraphProxy.sol:141: require(Address.isContract(_pendingImplementation), "Implementation must be a contract");
contracts/upgrades/GraphProxy.sol:157: require(msg.sender != _admin(), "Cannot fallback to proxy target");
contracts/upgrades/GraphUpgradeable.sol:24: require(msg.sender == _proxy.admin(), "Caller must be the proxy admin");
contracts/upgrades/GraphUpgradeable.sol:32: require(msg.sender == _implementation(), "Caller must be the implementation");
contracts/upgrades/GraphProxyStorage.sol:62: require(msg.sender == _admin(), "Caller must be admin");
contracts/gateway/L1GraphTokenGateway.sol:74: require(inbox != address(0), "INBOX_NOT_SET");
contracts/gateway/L1GraphTokenGateway.sol:78: require(msg.sender == address(bridge), "NOT_FROM_BRIDGE");
contracts/gateway/L1GraphTokenGateway.sol:82: require(l2ToL1Sender == l2Counterpart, "ONLY_COUNTERPART_GATEWAY");
contracts/gateway/L1GraphTokenGateway.sol:110: require(_inbox != address(0), "INVALID_INBOX");
contracts/gateway/L1GraphTokenGateway.sol:111: require(_l1Router != address(0), "INVALID_L1_ROUTER");
contracts/gateway/L1GraphTokenGateway.sol:122: require(_l2GRT != address(0), "INVALID_L2_GRT");
contracts/gateway/L1GraphTokenGateway.sol:132: require(_l2Counterpart != address(0), "INVALID_L2_COUNTERPART");
contracts/gateway/L1GraphTokenGateway.sol:142: require(_escrow != address(0) && Address.isContract(_escrow), "INVALID_ESCROW");
contracts/gateway/L1GraphTokenGateway.sol:153: require(_newWhitelisted != address(0), "INVALID_ADDRESS");
contracts/gateway/L1GraphTokenGateway.sol:154: require(!callhookWhitelist[_newWhitelisted], "ALREADY_WHITELISTED");
contracts/gateway/L1GraphTokenGateway.sol:165: require(_notWhitelisted != address(0), "INVALID_ADDRESS");
contracts/gateway/L1GraphTokenGateway.sol:166: require(callhookWhitelist[_notWhitelisted], "NOT_WHITELISTED");
contracts/gateway/L1GraphTokenGateway.sol:200: require(_l1Token == address(token), "TOKEN_NOT_GRT");
contracts/gateway/L1GraphTokenGateway.sol:201: require(_amount > 0, "INVALID_ZERO_AMOUNT");
contracts/gateway/L1GraphTokenGateway.sol:202: require(_to != address(0), "INVALID_DESTINATION");
contracts/gateway/L1GraphTokenGateway.sol:217: require(maxSubmissionCost > 0, "NO_SUBMISSION_COST");
contracts/gateway/L1GraphTokenGateway.sol:224: require(msg.value >= expectedEth, "WRONG_ETH_VALUE");
contracts/gateway/L1GraphTokenGateway.sol:271: require(_l1Token == address(token), "TOKEN_NOT_GRT");
contracts/gateway/L1GraphTokenGateway.sol:275: require(_amount <= escrowBalance, "BRIDGE_OUT_OF_FUNDS");
contracts/gateway/GraphTokenGateway.sol:31: require(_newPauseGuardian != address(0), "PauseGuardian must be set");
contracts/gateway/GraphTokenGateway.sol:40: require(!_paused, "Paused (contract)");
contracts/governance/Managed.sol:44: require(!controller.paused(), "Paused");
contracts/governance/Managed.sol:45: require(!controller.partialPaused(), "Partial-paused");
contracts/governance/Managed.sol:49: require(!controller.paused(), "Paused");
contracts/governance/Managed.sol:53: require(msg.sender == controller.getGovernor(), "Caller must be Controller governor");
contracts/governance/Managed.sol:57: require(msg.sender == address(controller), "Caller must be Controller");
contracts/governance/Managed.sol:104: require(_controller != address(0), "Controller must be set");
contracts/governance/Governed.sol:24: require(msg.sender == governor, "Only Governor can call");
contracts/governance/Governed.sol:41: require(_newGovernor != address(0), "Governor must be set");
I suggest replacing revert strings with custom errors.
Between solc 0.4.10 and 0.8.0, require() used REVERT (0xfd) opcode which refunded the remaining gas on failure while assert() used INVALID (0xfe) opcode which consumed all the supplied gas. (see https://docs.soliditylang.org/en/v0.8.1/control-structures.html#error-handling-assert-require-revert-and-exceptions). require() should be used for checking error conditions on inputs and return values while assert() should be used for invariant checking (properly functioning code should never reach a failing assert statement, unless there is a bug in your contract you should fix). From the current usage of assert, my guess is that they can be replaced with require unless a Panic really is intended.
contracts/upgrades/GraphProxy.sol:47: assert(ADMIN_SLOT == bytes32(uint256(keccak256("eip1967.proxy.admin")) - 1));
contracts/upgrades/GraphProxy.sol:48: assert(
contracts/upgrades/GraphProxy.sol:51: assert(
https://code4rena.com/reports/2022-05-bunker/#g-08-usage-of-assert-instead-of-require
This results in the keccak operation being performed whenever the variable is used, increasing gas costs relative to just storing the output hash. Changing to immutable will only perform hashing on contract deployment which will save gas.
contracts/l2/token/GraphTokenUpgradeable.sol:38: bytes32 private constant DOMAIN_NAME_HASH = keccak256("Graph Token");
contracts/l2/token/GraphTokenUpgradeable.sol:39: bytes32 private constant DOMAIN_VERSION_HASH = keccak256("0");
https://github.com/code-423n4/2021-10-slingshot-findings/issues/3
Use abi.encodepacked() instead of abi.encode()
contracts/l2/gateway/L2GraphTokenGateway.sol:174: return abi.encode(id);
contracts/l2/gateway/L2GraphTokenGateway.sol:275: abi.encode(0, _data) // we don't need to track exitNums (b/c we have no fast exits) so we always use 0
contracts/l2/token/GraphTokenUpgradeable.sol:88: abi.encode(PERMIT_TYPEHASH, _owner, _spender, _value, nonces[_owner], _deadline)
GraphTokenUpgradeable.sol:L162
contracts/l2/token/GraphTokenUpgradeable.sol:162: abi.encode(
contracts/gateway/L1GraphTokenGateway.sol:249: return abi.encode(seqNum);
contracts/gateway/L1GraphTokenGateway.sol:342: abi.encode(emptyBytes, _data)
Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.
Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.
contracts/upgrades/GraphProxy.sol:105: require(_newAdmin != address(0), "Cannot change the admin of a proxy to the zero address");
contracts/upgrades/GraphProxy.sol:141: require(Address.isContract(_pendingImplementation), "Implementation must be a contract");
contracts/upgrades/GraphUpgradeable.sol:32: require(msg.sender == _implementation(), "Caller must be the implementation");
contracts/governance/Managed.sol:53: require(msg.sender == controller.getGovernor(), "Caller must be Controller governor");
I suggest shortening the revert strings to fit in 32 bytes, or using custom errors as described next.
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)
Source Custom Errors in Solidity:
Use calldata instead of memory in external functions to save gas.
contracts/l2/gateway/L2GraphTokenGateway.sol:193: ) external returns (bytes memory) {
contracts/gateway/L1GraphTokenGateway.sol:198: ) external payable override notPaused returns (bytes memory) {
// 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.
Refer Here 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
contracts/l2/token/GraphTokenUpgradeable.sol:51: mapping(address => bool) private _minters;
contracts/gateway/L1GraphTokenGateway.sol:35: mapping(address => bool) public callhookWhitelist;
contracts/governance/Pausable.sol:8: bool internal _partialPaused;
contracts/governance/Pausable.sol:10: bool internal _paused;
https://code4rena.com/reports/2022-06-notional-coop#8-using-bools-for-storage-incurs-overhead
When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.
https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed
contracts/staking/IStaking.sol:47: uint32 _indexingRewardCut,
contracts/staking/IStaking.sol:48: uint32 _queryFeeCut,
contracts/staking/IStaking.sol:49: uint32 _cooldownBlocks
contracts/staking/IStakingData.sol:37: uint32 cooldownBlocks; // Blocks to wait before updating parameters
contracts/staking/IStakingData.sol:38: uint32 indexingRewardCut; // in PPM
contracts/staking/IStakingData.sol:39: uint32 queryFeeCut; // in PPM
contracts/l2/token/GraphTokenUpgradeable.sol:79: uint8 _v,
contracts/token/IGraphToken.sol:33: uint8 _v,
Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
contracts/curation/ICuration.sol:3:pragma solidity ^0.7.6;
contracts/curation/IGraphCurationToken.sol:3:pragma solidity ^0.7.6;
contracts/staking/IStaking.sol:3:pragma solidity >=0.6.12 <0.8.0;
contracts/staking/IStakingData.sol:3:pragma solidity >=0.6.12 <0.8.0;
contracts/l2/gateway/L2GraphTokenGateway.sol:3:pragma solidity ^0.7.6;
contracts/l2/token/L2GraphToken.sol:3:pragma solidity ^0.7.6;
contracts/l2/token/GraphTokenUpgradeable.sol:3:pragma solidity ^0.7.6;
contracts/upgrades/IGraphProxy.sol:3:pragma solidity ^0.7.6;
contracts/upgrades/GraphProxyAdmin.sol:3:pragma solidity ^0.7.6;
contracts/upgrades/GraphProxy.sol:3:pragma solidity ^0.7.6;
contracts/upgrades/GraphUpgradeable.sol:3:pragma solidity ^0.7.6;
contracts/upgrades/GraphProxyStorage.sol:3:pragma solidity ^0.7.6;
contracts/gateway/L1GraphTokenGateway.sol:3:pragma solidity ^0.7.6;
contracts/gateway/ICallhookReceiver.sol:9:pragma solidity ^0.7.6;
contracts/gateway/BridgeEscrow.sol:3:pragma solidity ^0.7.6;
contracts/gateway/GraphTokenGateway.sol:3:pragma solidity ^0.7.6;
contracts/epochs/IEpochManager.sol:3:pragma solidity ^0.7.6;
contracts/rewards/IRewardsManager.sol:3:pragma solidity ^0.7.6;
contracts/governance/Pausable.sol:3:pragma solidity ^0.7.6;
contracts/governance/Managed.sol:3:pragma solidity ^0.7.6;
contracts/governance/Governed.sol:3:pragma solidity ^0.7.6;
contracts/governance/IController.sol:3:pragma solidity >=0.6.12 <0.8.0;
contracts/token/IGraphToken.sol:3:pragma solidity ^0.7.6;
https://code4rena.com/reports/2022-06-notional-coop/#10-use-a-more-recent-version-of-solidity
Saves 6 gas per instance if using assembly to check for address(0)
e.g.
assembly { if iszero(_addr) { mstore(0x00, 'zero address') revert(0x00, 0x20) } }
contracts/l2/gateway/L2GraphTokenGateway.sol:98: require(_l2Router != address(0), "INVALID_L2_ROUTER");
contracts/l2/gateway/L2GraphTokenGateway.sol:108: require(_l1GRT != address(0), "INVALID_L1_GRT");
contracts/l2/gateway/L2GraphTokenGateway.sol:118: require(_l1Counterpart != address(0), "INVALID_L1_COUNTERPART");
contracts/l2/gateway/L2GraphTokenGateway.sol:148: require(_to != address(0), "INVALID_DESTINATION");
contracts/l2/token/L2GraphToken.sol:49: require(_owner != address(0), "Owner must be set");
contracts/l2/token/L2GraphToken.sol:60: require(_gw != address(0), "INVALID_GATEWAY");
contracts/l2/token/L2GraphToken.sol:70: require(_addr != address(0), "INVALID_L1_ADDRESS");
GraphTokenUpgradeable.sol:L106
contracts/l2/token/GraphTokenUpgradeable.sol:106: require(_account != address(0), "INVALID_MINTER");
contracts/upgrades/GraphProxy.sol:105: require(_newAdmin != address(0), "Cannot change the admin of a proxy to the zero address");
contracts/upgrades/GraphProxy.sol:143: _pendingImplementation != address(0) && msg.sender == _pendingImplementation,
contracts/gateway/L1GraphTokenGateway.sol:74: require(inbox != address(0), "INBOX_NOT_SET");
contracts/gateway/L1GraphTokenGateway.sol:110: require(_inbox != address(0), "INVALID_INBOX");
contracts/gateway/L1GraphTokenGateway.sol:111: require(_l1Router != address(0), "INVALID_L1_ROUTER");
contracts/gateway/L1GraphTokenGateway.sol:122: require(_l2GRT != address(0), "INVALID_L2_GRT");
contracts/gateway/L1GraphTokenGateway.sol:132: require(_l2Counterpart != address(0), "INVALID_L2_COUNTERPART");
contracts/gateway/L1GraphTokenGateway.sol:142: require(_escrow != address(0) && Address.isContract(_escrow), "INVALID_ESCROW");
contracts/gateway/L1GraphTokenGateway.sol:153: require(_newWhitelisted != address(0), "INVALID_ADDRESS");
contracts/gateway/L1GraphTokenGateway.sol:165: require(_notWhitelisted != address(0), "INVALID_ADDRESS");
contracts/gateway/L1GraphTokenGateway.sol:202: require(_to != address(0), "INVALID_DESTINATION");
contracts/gateway/GraphTokenGateway.sol:31: require(_newPauseGuardian != address(0), "PauseGuardian must be set");
contracts/governance/Managed.sol:104: require(_controller != address(0), "Controller must be set");
contracts/governance/Governed.sol:41: require(_newGovernor != address(0), "Governor must be set");
contracts/governance/Governed.sol:55: pendingGovernor != address(0) && msg.sender == pendingGovernor,
As notDelegated() is only used once in this contract (in function proxiableUUID()), it should get inlined to save gas:
contracts/l2/token/GraphTokenUpgradeable.sol:59: modifier onlyMinter() {
GraphTokenUpgradeable.sol:L132
contracts/l2/token/GraphTokenUpgradeable.sol:132: function mint(address _to, uint256 _amount) external onlyMinter {
contracts/l2/gateway/L2GraphTokenGateway.sol:68: modifier onlyL1Counterpart() {
contracts/l2/gateway/L2GraphTokenGateway.sol:232: ) external payable override notPaused onlyL1Counterpart nonReentrant {
contracts/governance/Managed.sol:71: modifier onlyController() {
contracts/governance/Managed.sol:95: function setController(address _controller) external onlyController {
contracts/gateway/L1GraphTokenGateway.sol:73: modifier onlyL2Counterpart() {
contracts/gateway/L1GraphTokenGateway.sol:269: ) external payable override notPaused onlyL2Counterpart {
contracts/gateway/GraphTokenGateway.sol:18: modifier onlyGovernorOrGuardian() {
contracts/gateway/GraphTokenGateway.sol:47: function setPaused(bool _newPaused) external onlyGovernorOrGuardian {