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: 9/62
Findings: 2
Award: $954.97
š Selected for report: 1
š Solo Findings: 0
š Selected for report: 0xSmartContract
Also found by: 0x1f8b, 0x4non, 0xNazgul, Bnke0x0, Chom, IllIllI, Josiah, Rahoz, RaymondFam, Trust, Waze, ajtra, bobirichman, brgltd, bulej93, c3phas, cccz, chrisdior4, delfin454000, fatherOfBlocks, gogo, ladboy233, mcwildy, mics, nicobevi, oyc_109, rbserver, rotcivegaf, zzzitron
602.1213 USDC - $602.12
| Issue | Instances | |
|---|---|---|
| [Lā01] | Upgradeable contract not initialized | 1 |
| [Lā02] | initialize() function does not use the initializer modifier | 1 |
| [Lā03] | require() should be used instead of assert() | 3 |
| [Lā04] | Missing checks for address(0x0) when assigning values to address state variables | 1 |
| [Lā05] | abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256() | 1 |
| [Lā06] | Upgradeable contract is missing a __gap[50] storage variable to allow for new storage variables in later versions | 4 |
Total: 11 instances over 6 issues
| Issue | Instances | |
|---|---|---|
| [Nā01] | Missing initializer modifier on constructor | 1 |
| [Nā02] | Missing initializer modifier | 1 |
| [Nā03] | The nonReentrant modifier should occur before all other modifiers | 2 |
| [Nā04] | require()/revert() statements should have descriptive reason strings | 4 |
| [Nā05] | public functions not called by the contract should be declared external instead | 6 |
| [Nā06] | Non-assembly method available | 1 |
| [Nā07] | constants should be defined rather than using magic numbers | 2 |
| [Nā08] | Cast is more restrictive than the type of the variable being assigned | 1 |
| [Nā09] | Use a more recent version of solidity | 3 |
| [Nā10] | Use a more recent version of solidity | 2 |
| [Nā11] | Expressions for constant values such as a call to keccak256(), should use immutable rather than constant | 4 |
| [Nā12] | Variable names that consist of all capital letters should be reserved for constant/immutable variables | 1 |
| [Nā13] | Non-library/interface files should use fixed compiler versions, not floating ones | 12 |
| [Nā14] | File is missing NatSpec | 6 |
| [Nā15] | NatSpec is incomplete | 6 |
| [Nā16] | Event is missing indexed fields | 18 |
| [Nā17] | Duplicated require()/revert() checks should be refactored to a modifier or function | 5 |
Total: 74 instances over 17 issues
Upgradeable contracts are initialized via an initializer function rather than by a constructor. Leaving such a contract uninitialized may lead to it being taken over by a malicious user
There is 1 instances of this issue:
File: contracts/l2/token/GraphTokenUpgradeable.sol /// @audit missing __ERC20Burnable_init() 28: contract GraphTokenUpgradeable is GraphUpgradeable, Governed, ERC20BurnableUpgradeable {
initialize() function does not use the initializer modifierWithout the modifier, the function may be called multiple times, overwriting prior initializations
There is 1 instance of this issue:
File: contracts/l2/gateway/L2GraphTokenGateway.sol 87: function initialize(address _controller) external onlyImpl {
require() should be used instead of assert()Prior to solidity version 0.8.0, hitting an assert consumes the remainder of the transaction's available gas rather than returning it, as require()/revert() do. assert() should be avoided even past solidity version 0.8.0 as its documentation states that "The assert function creates an error of type Panic(uint256). ... Properly functioning code should never create a Panic, not even on invalid external input. If this happens, then there is a bug in your contract which you should fix".
There are 3 instances of this issue:
File: contracts/upgrades/GraphProxy.sol 47: assert(ADMIN_SLOT == bytes32(uint256(keccak256("eip1967.proxy.admin")) - 1)); 48 assert( 49 IMPLEMENTATION_SLOT == bytes32(uint256(keccak256("eip1967.proxy.implementation")) - 1) 50: ); 51 assert( 52 PENDING_IMPLEMENTATION_SLOT == 53 bytes32(uint256(keccak256("eip1967.proxy.pendingImplementation")) - 1) 54: );
address(0x0) when assigning values to address state variablesThere is 1 instance of this issue:
File: contracts/governance/Governed.sol 32: governor = _initGovernor;
abi.encodePacked() should not be used with dynamic types when passing the result to a hash function such as keccak256()Use abi.encode() instead which will pad items to 32 bytes, which will prevent hash collisions (e.g. abi.encodePacked(0x123,0x456) => 0x123456 => abi.encodePacked(0x1,0x23456), but abi.encode(0x123,0x456) => 0x0...1230...456). "Unless there is a compelling reason, abi.encode should be preferred". If there is only one argument to abi.encodePacked() it can often be cast to bytes() or bytes32() instead.
If all arguments are strings and or bytes, bytes.concat() should be used instead
There is 1 instance of this issue:
File: contracts/governance/Managed.sol 174: bytes32 nameHash = keccak256(abi.encodePacked(_name));
__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 4 instances of this issue:
File: contracts/gateway/BridgeEscrow.sol 15: contract BridgeEscrow is GraphUpgradeable, Managed {
File: contracts/l2/gateway/L2GraphTokenGateway.sol 23: contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger, ReentrancyGuardUpgradeable {
File: contracts/l2/token/GraphTokenUpgradeable.sol 28: contract GraphTokenUpgradeable is GraphUpgradeable, Governed, ERC20BurnableUpgradeable {
File: contracts/l2/token/L2GraphToken.sol 15: contract L2GraphToken is GraphTokenUpgradeable, IArbToken {
initializer modifier on constructorOpenZeppelin recommends that the initializer modifier be applied to constructors in order to avoid potential griefs, social engineering, or exploits. Ensure that the modifier is applied to the implementation contract. If the default constructor is currently being used, it should be changed to be an explicit one with the modifier applied.
There is 1 instance of this issue:
File: contracts/l2/gateway/L2GraphTokenGateway.sol 23: contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger, ReentrancyGuardUpgradeable {
initializer modifierThe contract extends Initializable/ReentrancyGuardUpgradeable but does not use the initializer modifier anywhere
There is 1 instance of this issue:
File: contracts/l2/gateway/L2GraphTokenGateway.sol 23: contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger, ReentrancyGuardUpgradeable {
nonReentrant modifier should occur before all other modifiersThis is a best-practice to protect against reentrancy in other modifiers
There are 2 instances of this issue:
File: contracts/l2/gateway/L2GraphTokenGateway.sol 144: ) public payable override notPaused nonReentrant returns (bytes memory) { 232: ) external payable override notPaused onlyL1Counterpart nonReentrant {
require()/revert() statements should have descriptive reason stringsThere are 4 instances of this issue:
File: contracts/upgrades/GraphProxyAdmin.sol 34: require(success); 47: require(success); 59: require(success);
File: contracts/upgrades/GraphProxy.sol 133: require(success);
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 6 instances of this issue:
File: contracts/upgrades/GraphProxyAdmin.sol 30: function getProxyImplementation(IGraphProxy _proxy) public view returns (address) { 43: function getProxyPendingImplementation(IGraphProxy _proxy) public view returns (address) { 55: function getProxyAdmin(IGraphProxy _proxy) public view returns (address) { 68: function changeProxyAdmin(IGraphProxy _proxy, address _newAdmin) public onlyGovernor { 77: function upgrade(IGraphProxy _proxy, address _implementation) public onlyGovernor { 86: function acceptProxy(GraphUpgradeable _implementation, IGraphProxy _proxy) public onlyGovernor {
assembly{ id := chainid() } => uint256 id = block.chainid, assembly { size := extcodesize() } => uint256 size = address().code.length
There are some automated tools that will flag a project as having higher complexity if there is inline-assembly, so it's best to avoid using it where it's not necessary
There is 1 instance of this issue:
File: contracts/l2/token/GraphTokenUpgradeable.sol 199: id := chainid()
constants should be defined rather than using magic numbersEven assembly can benefit from using readable constants instead of hex/numeric literals
There are 2 instances of this issue:
File: contracts/upgrades/GraphProxy.sol /// @audit 0x40 161: let ptr := mload(0x40) /// @audit 0xffffffffffffffffffffffffffffffffffffffff 164: let impl := and(sload(IMPLEMENTATION_SLOT), 0xffffffffffffffffffffffffffffffffffffffff)
If address foo is being used in an expression such as IERC20 token = FooToken(foo), then the more specific cast to FooToken is a waste because the only thing the compiler will check for is that FooToken extends IERC20 - it won't check any of the function signatures. Therefore, it makes more sense to do IERC20 token = IERC20(token) or better yet FooToken token = FooToken(foo). The former may allow the file in which it's used to remove the import for FooToken
There is 1 instance of this issue:
File: contracts/governance/Managed.sol /// @audit IController vs address 105: controller = IController(_controller);
Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions
There are 3 instances of this issue:
File: contracts/gateway/L1GraphTokenGateway.sol 3: pragma solidity ^0.7.6;
File: contracts/l2/gateway/L2GraphTokenGateway.sol 3: pragma solidity ^0.7.6;
File: contracts/l2/token/L2GraphToken.sol 3: pragma solidity ^0.7.6;
Use a solidity version of at least 0.8.4 to get bytes.concat() instead of abi.encodePacked(<bytes>,<bytes>)
Use a solidity version of at least 0.8.12 to get string.concat() instead of abi.encodePacked(<str>,<str>)
There are 2 instances of this issue:
File: contracts/governance/Managed.sol 3: pragma solidity ^0.7.6;
File: contracts/l2/token/GraphTokenUpgradeable.sol 3: pragma solidity ^0.7.6;
keccak256(), should use immutable rather than constantWhile it doesn't save any gas because the compiler knows that developers often make this mistake, it's still best to use the right tool for the task at hand. There is a difference between constant variables and immutable variables, and they should each be used in their appropriate contexts. constants should be used for literal values written into the code, and immutable variables should be used for expressions, or values calculated in, or passed into the constructor.
There are 4 instances of this issue:
File: contracts/l2/token/GraphTokenUpgradeable.sol 34 bytes32 private constant DOMAIN_TYPE_HASH = 35 keccak256( 36 "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract,bytes32 salt)" 37: ); 38: bytes32 private constant DOMAIN_NAME_HASH = keccak256("Graph Token"); 39: bytes32 private constant DOMAIN_VERSION_HASH = keccak256("0"); 42 bytes32 private constant PERMIT_TYPEHASH = 43 keccak256( 44 "Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)" 45: );
constant/immutable variablesIf the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).
There is 1 instance of this issue:
File: contracts/l2/token/GraphTokenUpgradeable.sol 50: bytes32 private DOMAIN_SEPARATOR;
There are 12 instances of this issue:
File: contracts/gateway/BridgeEscrow.sol 3: pragma solidity ^0.7.6;
File: contracts/gateway/L1GraphTokenGateway.sol 3: pragma solidity ^0.7.6;
File: contracts/governance/Governed.sol 3: pragma solidity ^0.7.6;
File: contracts/governance/Managed.sol 3: pragma solidity ^0.7.6;
File: contracts/governance/Pausable.sol 3: pragma solidity ^0.7.6;
File: contracts/l2/gateway/L2GraphTokenGateway.sol 3: pragma solidity ^0.7.6;
File: contracts/l2/token/GraphTokenUpgradeable.sol 3: pragma solidity ^0.7.6;
File: contracts/l2/token/L2GraphToken.sol 3: pragma solidity ^0.7.6;
File: contracts/upgrades/GraphProxyAdmin.sol 3: pragma solidity ^0.7.6;
File: contracts/upgrades/GraphProxy.sol 3: pragma solidity ^0.7.6;
File: contracts/upgrades/GraphProxyStorage.sol 3: pragma solidity ^0.7.6;
File: contracts/upgrades/GraphUpgradeable.sol 3: pragma solidity ^0.7.6;
There are 6 instances of this issue:
File: contracts/curation/ICuration.sol
File: contracts/curation/IGraphCurationToken.sol
File: contracts/epochs/IEpochManager.sol
File: contracts/governance/IController.sol
File: contracts/token/IGraphToken.sol
File: contracts/upgrades/IGraphProxy.sol
There are 6 instances of this issue:
File: contracts/gateway/L1GraphTokenGateway.sol /// @audit Missing: '@param bytes' 252 /** 253 * @notice Receives withdrawn tokens from L2 254 * The equivalent tokens are released from escrow and sent to the destination. 255 * @dev can only accept transactions coming from the L2 GRT Gateway. 256 * The last parameter is unused but kept for compatibility with Arbitrum gateways, 257 * and the encoded exitNum is assumed to be 0. 258 * @param _l1Token L1 Address of the GRT contract (needed for compatibility with Arbitrum Gateway Router) 259 * @param _from Address of the sender 260 * @param _to Recepient address on L1 261 * @param _amount Amount of tokens transferred 262 */ 263 function finalizeInboundTransfer( 264 address _l1Token, 265 address _from, 266 address _to, 267 uint256 _amount, 268 bytes calldata // _data, contains exitNum, unused by this contract 269: ) external payable override notPaused onlyL2Counterpart {
File: contracts/governance/Managed.sol /// @audit Missing: '@param _nameHash' 157 /** 158 * @dev Resolve a contract address from the cache or the Controller if not found. 159 * @return Address of the contract 160 */ 161: function _resolveContract(bytes32 _nameHash) internal view returns (address) {
File: contracts/l2/gateway/L2GraphTokenGateway.sol /// @audit Missing: '@param uint256' 123 /** 124 * @notice Burns L2 tokens and initiates a transfer to L1. 125 * The tokens will be available on L1 only after the wait period (7 days) is over, 126 * and will require an Outbox.executeTransaction to finalize. 127 * Note that the caller must previously allow the gateway to spend the specified amount of GRT. 128 * @dev no additional callhook data is allowed. The two unused params are needed 129 * for compatibility with Arbitrum's gateway router. 130 * The function is payable for ITokenGateway compatibility, but msg.value must be zero. 131 * @param _l1Token L1 Address of GRT (needed for compatibility with Arbitrum Gateway Router) 132 * @param _to Recipient address on L1 133 * @param _amount Amount of tokens to burn 134 * @param _data Contains sender and additional data (always empty) to send to L1 135 * @return ID of the withdraw transaction 136 */ 137 function outboundTransfer( 138 address _l1Token, 139 address _to, 140 uint256 _amount, 141 uint256, // unused on L2 142 uint256, // unused on L2 143 bytes calldata _data 144: ) public payable override notPaused nonReentrant returns (bytes memory) {
File: contracts/upgrades/GraphProxyAdmin.sol /// @audit Missing: '@param _proxy' 25 /** 26 * @dev Returns the current implementation of a proxy. 27 * This is needed because only the proxy admin can query it. 28 * @return The address of the current implementation of the proxy. 29 */ 30: function getProxyImplementation(IGraphProxy _proxy) public view returns (address) { /// @audit Missing: '@param _proxy' 38 /** 39 * @dev Returns the pending implementation of a proxy. 40 * This is needed because only the proxy admin can query it. 41 * @return The address of the pending implementation of the proxy. 42 */ 43: function getProxyPendingImplementation(IGraphProxy _proxy) public view returns (address) { /// @audit Missing: '@param _proxy' 51 /** 52 * @dev Returns the admin of a proxy. Only the admin can query it. 53 * @return The address of the current admin of the proxy. 54 */ 55: function getProxyAdmin(IGraphProxy _proxy) public view returns (address) {
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 18 instances of this issue:
File: contracts/gateway/L1GraphTokenGateway.sol 56: event ArbitrumAddressesSet(address inbox, address l1Router); 58: event L2TokenAddressSet(address l2GRT); 60: event L2CounterpartAddressSet(address l2Counterpart); 62: event EscrowAddressSet(address escrow); 64: event AddedToCallhookWhitelist(address newWhitelisted); 66: event RemovedFromCallhookWhitelist(address notWhitelisted);
File: contracts/governance/Managed.sol 33: event ParameterUpdated(string param); 34: event SetController(address controller); 39: event ContractSynced(bytes32 indexed nameHash, address contractAddress);
File: contracts/governance/Pausable.sol 19: event PartialPauseChanged(bool isPaused); 20: event PauseChanged(bool isPaused);
File: contracts/l2/gateway/L2GraphTokenGateway.sol 58: event L2RouterSet(address l2Router); 60: event L1TokenAddressSet(address l1GRT); 62: event L1CounterpartAddressSet(address l1Counterpart);
File: contracts/l2/token/L2GraphToken.sol 24: event BridgeMinted(address indexed account, uint256 amount); 26: event BridgeBurned(address indexed account, uint256 amount); 28: event GatewaySet(address gateway); 30: event L1AddressSet(address l1Address);
require()/revert() checks should be refactored to a modifier or functionThe compiler will inline the function, which will avoid JUMP instructions usually associated with functions
There are 5 instances of this issue:
File: contracts/gateway/L1GraphTokenGateway.sol 271: require(_l1Token == address(token), "TOKEN_NOT_GRT");
File: contracts/governance/Managed.sol 49: require(!controller.paused(), "Paused");
File: contracts/l2/gateway/L2GraphTokenGateway.sol 233: require(_l1Token == l1GRT, "TOKEN_NOT_GRT"); 234: require(msg.value == 0, "INVALID_NONZERO_VALUE");
File: contracts/upgrades/GraphProxyAdmin.sol 47: require(success);
#0 - trust1995
2022-10-21T22:13:04Z
L-02 is dup of #149
š 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
352.8492 USDC - $352.85
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [Gā01] | State variables can be packed into fewer storage slots | 1 | - |
| [Gā02] | Avoid contract existence checks by using low level calls | 3 | 300 |
| [Gā03] | State variables should be cached in stack variables rather than re-reading them from storage | 10 | 970 |
| [Gā04] | internal functions only called once can be inlined to save gas | 3 | 60 |
| [Gā05] | require()/revert() strings longer than 32 bytes cost extra gas | 6 | - |
| [Gā06] | keccak256() should only need to be called on a specific string literal once | 6 | 252 |
| [Gā07] | Optimize names to save gas | 19 | 418 |
| [Gā08] | Using bools for storage incurs overhead | 4 | 68400 |
| [Gā09] | Use a more recent version of solidity | 20 | - |
| [Gā10] | Use a more recent version of solidity | 3 | - |
| [Gā11] | Using > 0 costs more gas than != 0 when used on a uint in a require() statement | 3 | 18 |
| [Gā12] | Splitting require() statements that use && saves gas | 3 | 9 |
| [Gā13] | Don't compare boolean expressions to boolean literals | 1 | 9 |
| [Gā14] | Stack variable used as a cheaper cache for a state variable is only used once | 4 | 12 |
| [Gā15] | require() or revert() statements that check input arguments should be at the top of the function | 1 | - |
| [Gā16] | Use custom errors rather than revert()/require() strings to save gas | 60 | - |
| [Gā17] | Functions guaranteed to revert when called by normal users can be marked payable | 32 | 672 |
Total: 179 instances over 17 issues with 7120 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
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/governance/Pausable.sol /// @audit Variable ordering with 3 slots instead of the current 4: /// uint256(32):lastPausePartialTime, uint256(32):lastPauseTime, address(20):pauseGuardian, bool(1):_partialPaused, bool(1):_paused 8: bool internal _partialPaused;
Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external function calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value. Similar behavior can be achieved in earlier versions by using low-level calls, since low level calls never check for contract existence
There are 3 instances of this issue:
File: contracts/gateway/BridgeEscrow.sol /// @audit approve() 29: graphToken().approve(_spender, type(uint256).max);
File: contracts/gateway/L1GraphTokenGateway.sol /// @audit bridge() 77: IBridge bridge = IInbox(inbox).bridge(); /// @audit l2ToL1Sender() 81: address l2ToL1Sender = IOutbox(bridge.activeOutbox()).l2ToL1Sender();
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replaces each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There are 10 instances of this issue:
File: contracts/governance/Governed.sol /// @audit governor on line 62 65: emit NewOwnership(oldGovernor, governor); /// @audit pendingGovernor on line 44 46: emit NewPendingOwnership(oldPendingGovernor, pendingGovernor); /// @audit pendingGovernor on line 55 55: pendingGovernor != address(0) && msg.sender == pendingGovernor, /// @audit pendingGovernor on line 55 60: address oldPendingGovernor = pendingGovernor; /// @audit pendingGovernor on line 60 62: governor = pendingGovernor; /// @audit pendingGovernor on line 63 66: emit NewPendingOwnership(oldPendingGovernor, pendingGovernor);
File: contracts/governance/Pausable.sol /// @audit _partialPaused on line 30 31: if (_partialPaused) { /// @audit _partialPaused on line 31 34: emit PartialPauseChanged(_partialPaused); /// @audit _paused on line 44 45: if (_paused) { /// @audit _paused on line 45 48: emit PauseChanged(_paused);
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 3 instances of this issue:
File: contracts/governance/Managed.sol 43 function _notPartialPaused() internal view { 44: require(!controller.paused(), "Paused"); 52 function _onlyGovernor() internal view { 53: require(msg.sender == controller.getGovernor(), "Caller must be Controller governor"); 56 function _onlyController() internal view { 57: require(msg.sender == address(controller), "Caller must be Controller");
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 6 instances of this issue:
File: contracts/gateway/GraphTokenGateway.sol 19 require( 20 msg.sender == controller.getGovernor() || msg.sender == pauseGuardian, 21 "Only Governor or Guardian can call" 22: );
File: contracts/governance/Managed.sol 53: require(msg.sender == controller.getGovernor(), "Caller must be Controller governor");
File: contracts/upgrades/GraphProxy.sol 105: require(_newAdmin != address(0), "Cannot change the admin of a proxy to the zero address"); 141: require(Address.isContract(_pendingImplementation), "Implementation must be a contract"); 142 require( 143 _pendingImplementation != address(0) && msg.sender == _pendingImplementation, 144 "Caller must be the pending implementation" 145: );
File: contracts/upgrades/GraphUpgradeable.sol 32: require(msg.sender == _implementation(), "Caller must be the implementation");
keccak256() should only need to be called on a specific string literal onceIt should be saved to an immutable variable, and the variable used instead. If the hash is being used as a part of a function selector, the cast to bytes4 should also only be done once
There are 6 instances of this issue:
File: contracts/governance/Managed.sol 114: return ICuration(_resolveContract(keccak256("Curation"))); 122: return IEpochManager(_resolveContract(keccak256("EpochManager"))); 130: return IRewardsManager(_resolveContract(keccak256("RewardsManager"))); 138: return IStaking(_resolveContract(keccak256("Staking"))); 146: return IGraphToken(_resolveContract(keccak256("GraphToken"))); 154: return ITokenGateway(_resolveContract(keccak256("GraphTokenGateway")));
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 19 instances of this issue:
File: contracts/curation/ICuration.sol /// @audit setDefaultReserveRatio(), setMinimumCurationDeposit(), setCurationTaxPercentage(), setCurationTokenMaster(), mint(), burn(), collect(), isCurated(), getCuratorSignal(), getCurationPoolSignal(), getCurationPoolTokens(), tokensToSignal(), signalToTokens(), curationTaxPercentage() 7: interface ICuration {
File: contracts/curation/IGraphCurationToken.sol /// @audit initialize(), burnFrom(), mint() 7: interface IGraphCurationToken is IERC20Upgradeable {
File: contracts/epochs/IEpochManager.sol /// @audit setEpochLength(), runEpoch(), isCurrentEpochRun(), blockNum(), blockHash(), currentEpoch(), currentEpochBlock(), currentEpochBlockSinceStart(), epochsSince(), epochsSinceUpdate() 5: interface IEpochManager {
File: contracts/gateway/BridgeEscrow.sol /// @audit initialize(), approveAll(), revokeAll() 15: contract BridgeEscrow is GraphUpgradeable, Managed {
File: contracts/gateway/GraphTokenGateway.sol /// @audit setPauseGuardian(), setPaused(), paused() 14: abstract contract GraphTokenGateway is GraphUpgradeable, Pausable, Managed, ITokenGateway {
File: contracts/gateway/ICallhookReceiver.sol /// @audit onTokenTransfer() 11: interface ICallhookReceiver {
File: contracts/gateway/L1GraphTokenGateway.sol /// @audit initialize(), setArbitrumAddresses(), setL2TokenAddress(), setL2CounterpartAddress(), setEscrowAddress(), addToCallhookWhitelist(), removeFromCallhookWhitelist(), getOutboundCalldata() 21: contract L1GraphTokenGateway is GraphTokenGateway, L1ArbitrumMessenger {
File: contracts/governance/IController.sol /// @audit getGovernor(), setContractProxy(), unsetContractProxy(), updateController(), getContractProxy(), setPartialPaused(), setPaused(), setPauseGuardian(), paused(), partialPaused() 5: interface IController {
File: contracts/governance/Managed.sol /// @audit syncAllContracts() 23: contract Managed {
File: contracts/l2/gateway/L2GraphTokenGateway.sol /// @audit initialize(), setL2Router(), setL1TokenAddress(), setL1CounterpartAddress(), outboundTransfer(), getOutboundCalldata() 23: contract L2GraphTokenGateway is GraphTokenGateway, L2ArbitrumMessenger, ReentrancyGuardUpgradeable {
File: contracts/l2/token/GraphTokenUpgradeable.sol /// @audit addMinter(), removeMinter(), renounceMinter(), mint(), isMinter() 28: contract GraphTokenUpgradeable is GraphUpgradeable, Governed, ERC20BurnableUpgradeable {
File: contracts/l2/token/L2GraphToken.sol /// @audit initialize(), setGateway(), setL1Address() 15: contract L2GraphToken is GraphTokenUpgradeable, IArbToken {
File: contracts/rewards/IRewardsManager.sol /// @audit setIssuanceRate(), setMinimumSubgraphSignal(), setSubgraphAvailabilityOracle(), setDenied(), setDeniedMany(), isDenied(), getNewRewardsPerSignal(), getAccRewardsPerSignal(), getAccRewardsForSubgraph(), getAccRewardsPerAllocatedToken(), getRewards(), updateAccRewardsPerSignal(), takeRewards(), onSubgraphSignalUpdate(), onSubgraphAllocationUpdate() 5: interface IRewardsManager {
File: contracts/staking/IStaking.sol /// @audit setMinimumIndexerStake(), setThawingPeriod(), setCurationPercentage(), setProtocolPercentage(), setChannelDisputeEpochs(), setMaxAllocationEpochs(), setRebateRatio(), setDelegationRatio(), setDelegationParameters(), setDelegationParametersCooldown(), setDelegationUnbondingPeriod(), setDelegationTaxPercentage(), setSlasher(), setAssetHolder(), setOperator(), isOperator(), stake(), stakeTo(), unstake(), slash(), withdraw(), setRewardsDestination(), delegate(), undelegate(), withdrawDelegated(), allocate(), allocateFrom(), closeAllocation(), closeAllocationMany(), closeAndAllocate(), collect(), claim(), claimMany(), hasStake(), getIndexerStakedTokens(), getIndexerCapacity(), getAllocation(), getAllocationState(), isAllocation(), getSubgraphAllocatedTokens(), getDelegation(), isDelegator() 8: interface IStaking is IStakingData {
File: contracts/token/IGraphToken.sol /// @audit burn(), burnFrom(), mint(), addMinter(), removeMinter(), renounceMinter(), isMinter() 7: interface IGraphToken is IERC20 {
File: contracts/upgrades/GraphProxyAdmin.sol /// @audit getProxyImplementation(), getProxyPendingImplementation(), getProxyAdmin(), changeProxyAdmin(), upgrade(), acceptProxy(), acceptProxyAndCall() 17: contract GraphProxyAdmin is Governed {
File: contracts/upgrades/GraphProxy.sol /// @audit implementation(), pendingImplementation(), setAdmin(), upgradeTo(), acceptUpgrade(), acceptUpgradeAndCall() 16: contract GraphProxy is GraphProxyStorage {
File: contracts/upgrades/GraphUpgradeable.sol /// @audit acceptProxy(), acceptProxyAndCall() 11: contract GraphUpgradeable {
File: contracts/upgrades/IGraphProxy.sol /// @audit setAdmin(), implementation(), pendingImplementation(), upgradeTo(), acceptUpgrade(), acceptUpgradeAndCall() 5: interface IGraphProxy {
bools for storage incurs overhead// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past
There are 4 instances of this issue:
File: contracts/gateway/L1GraphTokenGateway.sol 35: mapping(address => bool) public callhookWhitelist;
File: contracts/governance/Pausable.sol 8: bool internal _partialPaused; 10: bool internal _paused;
File: contracts/l2/token/GraphTokenUpgradeable.sol 51: mapping(address => bool) private _minters;
Use a solidity version of at least 0.8.0 to get overflow protection without SafeMath
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
There are 20 instances of this issue:
File: contracts/curation/ICuration.sol 3: pragma solidity ^0.7.6;
File: contracts/curation/IGraphCurationToken.sol 3: pragma solidity ^0.7.6;
File: contracts/epochs/IEpochManager.sol 3: pragma solidity ^0.7.6;
File: contracts/gateway/BridgeEscrow.sol 3: pragma solidity ^0.7.6;
File: contracts/gateway/GraphTokenGateway.sol 3: pragma solidity ^0.7.6;
File: contracts/gateway/ICallhookReceiver.sol 9: pragma solidity ^0.7.6;
File: contracts/gateway/L1GraphTokenGateway.sol 3: pragma solidity ^0.7.6;
File: contracts/governance/Governed.sol 3: pragma solidity ^0.7.6;
File: contracts/governance/Managed.sol 3: pragma solidity ^0.7.6;
File: contracts/governance/Pausable.sol 3: pragma solidity ^0.7.6;
File: contracts/l2/gateway/L2GraphTokenGateway.sol 3: pragma solidity ^0.7.6;
File: contracts/l2/token/GraphTokenUpgradeable.sol 3: pragma solidity ^0.7.6;
File: contracts/l2/token/L2GraphToken.sol 3: pragma solidity ^0.7.6;
File: contracts/rewards/IRewardsManager.sol 3: pragma solidity ^0.7.6;
File: contracts/token/IGraphToken.sol 3: pragma solidity ^0.7.6;
File: contracts/upgrades/GraphProxyAdmin.sol 3: pragma solidity ^0.7.6;
File: contracts/upgrades/GraphProxy.sol 3: pragma solidity ^0.7.6;
File: contracts/upgrades/GraphProxyStorage.sol 3: pragma solidity ^0.7.6;
File: contracts/upgrades/GraphUpgradeable.sol 3: pragma solidity ^0.7.6;
File: contracts/upgrades/IGraphProxy.sol 3: pragma solidity ^0.7.6;
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
There are 3 instances of this issue:
File: contracts/governance/IController.sol 3: pragma solidity >=0.6.12 <0.8.0;
File: contracts/staking/IStakingData.sol 3: pragma solidity >=0.6.12 <0.8.0;
File: contracts/staking/IStaking.sol 3: pragma solidity >=0.6.12 <0.8.0;
> 0 costs more gas than != 0 when used on a uint in a require() statementThis change saves 6 gas per instance. The optimization works until solidity version 0.8.13 where there is a regression in gas costs.
There are 3 instances of this issue:
File: contracts/gateway/L1GraphTokenGateway.sol 201: require(_amount > 0, "INVALID_ZERO_AMOUNT"); 217: require(maxSubmissionCost > 0, "NO_SUBMISSION_COST");
File: contracts/l2/gateway/L2GraphTokenGateway.sol 146: require(_amount > 0, "INVALID_ZERO_AMOUNT");
require() statements that use && saves gasSee this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper by 3 gas
There are 3 instances of this issue:
File: contracts/gateway/L1GraphTokenGateway.sol 142: require(_escrow != address(0) && Address.isContract(_escrow), "INVALID_ESCROW");
File: contracts/governance/Governed.sol 54 require( 55 pendingGovernor != address(0) && msg.sender == pendingGovernor, 56 "Caller must be pending governor" 57: );
File: contracts/upgrades/GraphProxy.sol 142 require( 143 _pendingImplementation != address(0) && msg.sender == _pendingImplementation, 144 "Caller must be the pending implementation" 145: );
if (<x> == true) => if (<x>), if (<x> == false) => if (!<x>)
There is 1 instance of this issue:
File: contracts/gateway/L1GraphTokenGateway.sol 214: extraData.length == 0 || callhookWhitelist[msg.sender] == true,
If the variable is only accessed once, it's cheaper to use the state variable directly that one time, and save the 3 gas the extra stack assignment would spend
There are 4 instances of this issue:
File: contracts/governance/Governed.sol 43: address oldPendingGovernor = pendingGovernor; 59: address oldGovernor = governor; 60: address oldPendingGovernor = pendingGovernor;
File: contracts/governance/Pausable.sol 56: address oldPauseGuardian = pauseGuardian;
require() or revert() statements that check input arguments should be at the top of the functionChecks that involve constants should come before checks that involve state variables, function calls, and calculations. By doing these checks first, the function is able to revert before wasting a Gcoldsload (2100 gas*) in a function that may ultimately revert in the unhappy case.
There is 1 instance of this issue:
File: contracts/gateway/L1GraphTokenGateway.sol /// @audit expensive op on line 199 201: require(_amount > 0, "INVALID_ZERO_AMOUNT");
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 60 instances of this issue:
File: contracts/gateway/GraphTokenGateway.sol 19 require( 20 msg.sender == controller.getGovernor() || msg.sender == pauseGuardian, 21 "Only Governor or Guardian can call" 22: ); 31: require(_newPauseGuardian != address(0), "PauseGuardian must be set"); 40: require(!_paused, "Paused (contract)");
File: contracts/gateway/L1GraphTokenGateway.sol 74: require(inbox != address(0), "INBOX_NOT_SET"); 78: require(msg.sender == address(bridge), "NOT_FROM_BRIDGE"); 82: require(l2ToL1Sender == l2Counterpart, "ONLY_COUNTERPART_GATEWAY"); 110: require(_inbox != address(0), "INVALID_INBOX"); 111: require(_l1Router != address(0), "INVALID_L1_ROUTER"); 122: require(_l2GRT != address(0), "INVALID_L2_GRT"); 132: require(_l2Counterpart != address(0), "INVALID_L2_COUNTERPART"); 142: require(_escrow != address(0) && Address.isContract(_escrow), "INVALID_ESCROW"); 153: require(_newWhitelisted != address(0), "INVALID_ADDRESS"); 154: require(!callhookWhitelist[_newWhitelisted], "ALREADY_WHITELISTED"); 165: require(_notWhitelisted != address(0), "INVALID_ADDRESS"); 166: require(callhookWhitelist[_notWhitelisted], "NOT_WHITELISTED"); 200: require(_l1Token == address(token), "TOKEN_NOT_GRT"); 201: require(_amount > 0, "INVALID_ZERO_AMOUNT"); 202: require(_to != address(0), "INVALID_DESTINATION"); 213 require( 214 extraData.length == 0 || callhookWhitelist[msg.sender] == true, 215 "CALL_HOOK_DATA_NOT_ALLOWED" 216: ); 217: require(maxSubmissionCost > 0, "NO_SUBMISSION_COST"); 224: require(msg.value >= expectedEth, "WRONG_ETH_VALUE"); 271: require(_l1Token == address(token), "TOKEN_NOT_GRT"); 275: require(_amount <= escrowBalance, "BRIDGE_OUT_OF_FUNDS");
File: contracts/governance/Governed.sol 24: require(msg.sender == governor, "Only Governor can call"); 41: require(_newGovernor != address(0), "Governor must be set"); 54 require( 55 pendingGovernor != address(0) && msg.sender == pendingGovernor, 56 "Caller must be pending governor" 57: );
File: contracts/governance/Managed.sol 44: require(!controller.paused(), "Paused"); 45: require(!controller.partialPaused(), "Partial-paused"); 49: require(!controller.paused(), "Paused"); 53: require(msg.sender == controller.getGovernor(), "Caller must be Controller governor"); 57: require(msg.sender == address(controller), "Caller must be Controller"); 104: require(_controller != address(0), "Controller must be set");
File: contracts/l2/gateway/L2GraphTokenGateway.sol 69 require( 70 msg.sender == AddressAliasHelper.applyL1ToL2Alias(l1Counterpart), 71 "ONLY_COUNTERPART_GATEWAY" 72: ); 98: require(_l2Router != address(0), "INVALID_L2_ROUTER"); 108: require(_l1GRT != address(0), "INVALID_L1_GRT"); 118: require(_l1Counterpart != address(0), "INVALID_L1_COUNTERPART"); 145: require(_l1Token == l1GRT, "TOKEN_NOT_GRT"); 146: require(_amount > 0, "INVALID_ZERO_AMOUNT"); 147: require(msg.value == 0, "INVALID_NONZERO_VALUE"); 148: require(_to != address(0), "INVALID_DESTINATION"); 153: require(outboundCalldata.extraData.length == 0, "CALL_HOOK_DATA_NOT_ALLOWED"); 233: require(_l1Token == l1GRT, "TOKEN_NOT_GRT"); 234: require(msg.value == 0, "INVALID_NONZERO_VALUE");
File: contracts/l2/token/GraphTokenUpgradeable.sol 60: require(isMinter(msg.sender), "Only minter can call"); 94: require(_owner == recoveredAddress, "GRT: invalid permit"); 95: require(_deadline == 0 || block.timestamp <= _deadline, "GRT: expired permit"); 106: require(_account != address(0), "INVALID_MINTER"); 115: require(_minters[_account], "NOT_A_MINTER"); 123: require(_minters[msg.sender], "NOT_A_MINTER");
File: contracts/l2/token/L2GraphToken.sol 36: require(msg.sender == gateway, "NOT_GATEWAY"); 49: require(_owner != address(0), "Owner must be set"); 60: require(_gw != address(0), "INVALID_GATEWAY"); 70: require(_addr != address(0), "INVALID_L1_ADDRESS");
File: contracts/upgrades/GraphProxy.sol 105: require(_newAdmin != address(0), "Cannot change the admin of a proxy to the zero address"); 141: require(Address.isContract(_pendingImplementation), "Implementation must be a contract"); 142 require( 143 _pendingImplementation != address(0) && msg.sender == _pendingImplementation, 144 "Caller must be the pending implementation" 145: ); 157: require(msg.sender != _admin(), "Cannot fallback to proxy target");
File: contracts/upgrades/GraphProxyStorage.sol 62: require(msg.sender == _admin(), "Caller must be admin");
File: contracts/upgrades/GraphUpgradeable.sol 24: require(msg.sender == _proxy.admin(), "Caller must be the proxy admin"); 32: require(msg.sender == _implementation(), "Caller must be the implementation");
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 32 instances of this issue:
File: contracts/gateway/BridgeEscrow.sol 20: function initialize(address _controller) external onlyImpl { 28: function approveAll(address _spender) external onlyGovernor { 36: function revokeAll(address _spender) external onlyGovernor {
File: contracts/gateway/GraphTokenGateway.sol 30: function setPauseGuardian(address _newPauseGuardian) external onlyGovernor { 47: function setPaused(bool _newPaused) external onlyGovernorOrGuardian {
File: contracts/gateway/L1GraphTokenGateway.sol 99: function initialize(address _controller) external onlyImpl { 109: function setArbitrumAddresses(address _inbox, address _l1Router) external onlyGovernor { 121: function setL2TokenAddress(address _l2GRT) external onlyGovernor { 131: function setL2CounterpartAddress(address _l2Counterpart) external onlyGovernor { 141: function setEscrowAddress(address _escrow) external onlyGovernor { 152: function addToCallhookWhitelist(address _newWhitelisted) external onlyGovernor { 164: function removeFromCallhookWhitelist(address _notWhitelisted) external onlyGovernor {
File: contracts/governance/Governed.sol 40: function transferOwnership(address _newGovernor) external onlyGovernor {
File: contracts/governance/Managed.sol 95: function setController(address _controller) external onlyController {
File: contracts/l2/gateway/L2GraphTokenGateway.sol 87: function initialize(address _controller) external onlyImpl { 97: function setL2Router(address _l2Router) external onlyGovernor { 107: function setL1TokenAddress(address _l1GRT) external onlyGovernor { 117: function setL1CounterpartAddress(address _l1Counterpart) external onlyGovernor {
File: contracts/l2/token/GraphTokenUpgradeable.sol 105: function addMinter(address _account) external onlyGovernor { 114: function removeMinter(address _account) external onlyGovernor { 132: function mint(address _to, uint256 _amount) external onlyMinter {
File: contracts/l2/token/L2GraphToken.sol 48: function initialize(address _owner) external onlyImpl { 59: function setGateway(address _gw) external onlyGovernor { 69: function setL1Address(address _addr) external onlyGovernor { 80: function bridgeMint(address _account, uint256 _amount) external override onlyGateway { 90: function bridgeBurn(address _account, uint256 _amount) external override onlyGateway {
File: contracts/upgrades/GraphProxyAdmin.sol 68: function changeProxyAdmin(IGraphProxy _proxy, address _newAdmin) public onlyGovernor { 77: function upgrade(IGraphProxy _proxy, address _implementation) public onlyGovernor { 86: function acceptProxy(GraphUpgradeable _implementation, IGraphProxy _proxy) public onlyGovernor { 96 function acceptProxyAndCall( 97 GraphUpgradeable _implementation, 98 IGraphProxy _proxy, 99 bytes calldata _data 100: ) external onlyGovernor {
File: contracts/upgrades/GraphUpgradeable.sol 50: function acceptProxy(IGraphProxy _proxy) external onlyProxyAdmin(_proxy) { 59 function acceptProxyAndCall(IGraphProxy _proxy, bytes calldata _data) 60 external 61: onlyProxyAdmin(_proxy)
#0 - tmigone
2022-10-21T20:06:51Z
We've found this submission to be of high quality.