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: 15/62
Findings: 2
Award: $321.70
🌟 Selected for report: 0
🚀 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
50.2765 USDC - $50.28
approve which is subject to a known front-running attack and failing for certain token implementations that do not return a boolean value. Consider using safeApprove instead (or better: safeIncreaseAllowance()/safeDecreaseAllowance()):
File: /contracts/gateway/BridgeEscrow.sol 28: function approveAll(address _spender) external onlyGovernor { 29: graphToken().approve(_spender, type(uint256).max); 30: }
approve() not checkedNot all IERC20 implementations revert() when there’s a failure in approve(). The function signature has a boolean return value and they indicate errors that way instead. By not checking the return value, operations that should have marked as failed, may potentially go through without actually approving anything
File: /contracts/gateway/BridgeEscrow.sol 28: function approveAll(address _spender) external onlyGovernor { 29: graphToken().approve(_spender, type(uint256).max); 30:
decreaseAllowance() not checkedFile: /contracts/gateway/BridgeEscrow.sol 36: function revokeAll(address _spender) external onlyGovernor { 37: IGraphToken grt = graphToken(); 38: grt.decreaseAllowance(_spender, grt.allowance(address(this), _spender)); 39: }
The signature of the function decreaseAllowance() indicates that it should return a boolean value. see the following
https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/token/IGraphToken.sol#L42
File: /contracts/token/IGraphToken.sol 42: function decreaseAllowance(address spender, uint256 subtractedValue) external returns (bool);
However, our revokeAll() function does not check this return value
Some tokens do not implement the ERC20 standard properly but are still accepted by most code that accepts ERC20 tokens. For example Tether (USDT)'s transfer() and transferFrom() functions do not return booleans as the specification requires, and instead have no return value. When these sorts of tokens are cast to IERC20, their function signatures do not match and therefore the calls made, revert. Use OpenZeppelin’s SafeERC20's safeTransfer()/safeTransferFrom() instead or check the return value.
Bad:
IERC20(token).transferFrom(msg.sender, address(this), amount);
Good (using OpenZeppelin's SafeERC20):
import {SafeERC20} from "openzeppelin/token/utils/SafeERC20.sol"; // ... IERC20(token).safeTransferFrom(msg.sender, address(this), amount);
Good (using require):
bool success = IERC20(token).transferFrom(msg.sender, address(this), amount); require(success, "ERC20 transfer failed");
File: /contracts/gateway/L1GraphTokenGateway.sol 235: token.transferFrom(from, escrow, _amount); 276: token.transferFrom(escrow, _to, _amount);
Consider using safeTransfer/safeTransferFrom or wrap in a require statement here:
File: /contracts/governance/Pausable.sol //@audit: pauseGuardian = newPauseGuardian; 55: function _setPauseGuardian(address newPauseGuardian) internal { 56: address oldPauseGuardian = pauseGuardian; 57: pauseGuardian = newPauseGuardian; 58: emit NewPauseGuardian(oldPauseGuardian, pauseGuardian); 59: }
File: /contracts/governance/Governed.sol //@audit: governor = _initGovernor 31: function _initialize(address _initGovernor) internal { 32: governor = _initGovernor; 33: }
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
File: /contracts/l2/token/GraphTokenUpgradeable.sol 83: bytes32 digest = keccak256( 84: abi.encodePacked( 85: "\x19\x01", 86: DOMAIN_SEPARATOR, 87: keccak256( 88: abi.encode(PERMIT_TYPEHASH, _owner, _spender, _value, nonces[_owner], _deadline) 89: ) 90: ) 91: );
assembly{ id := chainid() } => uint256 id = block.chainid
File: /contracts/l2/token/GraphTokenUpgradeable.sol 195: function _getChainID() private pure returns (uint256) { 196: uint256 id; 197: // solhint-disable-next-line no-inline-assembly 198: assembly { 199: id := chainid() 200: } 201: return id; 202: }
Contracts are allowed to override their parents' functions and change the visibility from external to public. Functions marked by external use call data to read arguments, where public will first allocate in local memory and then load them.
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 {
The following function is not being used anywhere https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/governance/Managed.sol#L153-L155
File: /contracts/governance/Managed.sol 153: function graphTokenGateway() internal view returns (ITokenGateway) { 154: return ITokenGateway(_resolveContract(keccak256("GraphTokenGateway"))); 155: }
Contracts should be deployed with the same compiler version and flags that they have been tested the most with. Locking the pragma helps ensure that contracts do not accidentally get deployed using, for example, the latest compiler which may have higher risks of undiscovered bugs. Contracts may also be deployed by others and the pragma indicates the compiler version intended by the original authors.
File: /contracts/gateway/BridgeEscrow.sol 3:pragma solidity ^0.7.6;
File: /contracts/upgrades/GraphUpgradeable.sol 3:pragma solidity ^0.7.6;
File: /contracts/governance/Governed.sol 3:pragma solidity ^0.7.6;
File: /contracts/governance/Pausable.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/GraphProxyStorage.sol 3:pragma solidity ^0.7.6;
File: /contracts/upgrades/GraphProxy.sol 3:pragma solidity ^0.7.6;
File: /contracts/governance/Managed.sol 3:pragma solidity ^0.7.6;
File: /contracts/l2/token/GraphTokenUpgradeable.sol 3:pragma solidity ^0.7.6;
File: /contracts/l2/gateway/L2GraphTokenGateway.sol 3:pragma solidity ^0.7.6;
File: /contracts/gateway/L1GraphTokenGateway.sol 3:pragma solidity ^0.7.6;
File: /contracts/gateway/GraphTokenGateway.sol 3:pragma solidity ^0.7.6;
File: /contracts/l2/token/GraphTokenUpgradeable.sol 50: bytes32 private DOMAIN_SEPARATOR;
File: /contracts/upgrades/GraphProxy.sol //@audit: Missing @param _newAdmin 104: function setAdmin(address _newAdmin) external ifAdmin { //@audit: Missing @param data 129: function acceptUpgradeAndCall(bytes calldata data) external ifAdminOrPendingImpl {
File: /contracts/upgrades/GraphProxyAdmin.sol //@audit: Missing @param _proxy 30: function getProxyImplementation(IGraphProxy _proxy) public view returns (address) { //@audit: Missing @param _proxy 43: function getProxyPendingImplementation(IGraphProxy _proxy) public view returns (address) { //@audit: Missing @param _proxy 55: function getProxyAdmin(IGraphProxy _proxy) public view returns (address) {
File: /contracts/upgrades/GraphUpgradeable.sol //@audit: Missing @param _proxy 50: function acceptProxy(IGraphProxy _proxy) external onlyProxyAdmin(_proxy) { //@audit: Missing @param _proxy, @param _data 59: function acceptProxyAndCall(IGraphProxy _proxy, bytes calldata _data)
File: /contracts/governance/Governed.sol 31: function _initialize(address _initGovernor) internal { 32: governor = _initGovernor; 33: }
require()/revert() statements should have descriptive reason strings. https://github.com/code-423n4/2022-10-thegraph/blob/309a188f7215fa42c745b136357702400f91b4ff/contracts/upgrades/GraphProxyAdmin.sol#L34
File: /contracts/upgrades/GraphProxyAdmin.sol 34: require(success); 47: require(success); 59: require(success);
File: /contracts/upgrades/GraphProxy.sol 133: require(success);
This is a best-practice to protect against reentrancy in other modifiers
File: /contracts/l2/gateway/L2GraphTokenGateway.sol 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/l2/gateway/L2GraphTokenGateway.sol 226: function finalizeInboundTransfer( 227: address _l1Token, 228: address _from, 229: address _to, 230: uint256 _amount, 231: bytes calldata _data 232: ) external payable override notPaused onlyL1Counterpart nonReentrant {
#0 - pcarranzav
2022-10-18T19:58:05Z
I'd dispute the validity of some of the claims here, but "The nonReentrant modifier should occur before all other modifiers" is a good catch
🌟 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
271.4225 USDC - $271.42
NB: Some functions have been truncated where neccessary to just show affected parts of the code
The code can be optimized by minimizing the number of SLOADs.
SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.
File: /contracts/gateway/L1GraphTokenGateway.sol 73: modifier onlyL2Counterpart() { 74: require(inbox != address(0), "INBOX_NOT_SET"); //@audit: SLOAD 1 76: // a message coming from the counterpart gateway was executed by the bridge 77: IBridge bridge = IInbox(inbox).bridge(); //@audit: SLOAD 2 78: require(msg.sender == address(bridge), "NOT_FROM_BRIDGE"); 84: }
File: /contracts/gateway/L1GraphTokenGateway.sol 263: function finalizeInboundTransfer( 273: uint256 escrowBalance = token.balanceOf(escrow); //@audit: SLOAD 1 274: // If the bridge doesn't have enough tokens, something's very wrong! 275: require(_amount <= escrowBalance, "BRIDGE_OUT_OF_FUNDS"); 276: token.transferFrom(escrow, _to, _amount); //@audit: SLOAD 2 279: }
File: /contracts/l2/gateway/L2GraphTokenGateway.sol 137: function outboundTransfer( 144: ) public payable override notPaused nonReentrant returns (bytes memory) { 145: require(_l1Token == l1GRT, "TOKEN_NOT_GRT"); //@audit: SLOAD 1 156: L2GraphToken(calculateL2TokenAddress(l1GRT)).bridgeBurn(outboundCalldata.from, _amount);//@audit: SLOAD 2 175: }
File: /contracts/l2/gateway/L2GraphTokenGateway.sol 226: function finalizeInboundTransfer( 232: ) external payable override notPaused onlyL1Counterpart nonReentrant { 233: require(_l1Token == l1GRT, "TOKEN_NOT_GRT"); //@audit: SLOAD 1 236: L2GraphToken(calculateL2TokenAddress(l1GRT)).bridgeMint(_to, _amount);//@audit: SLOAD 2 248: }
File: /contracts/governance/Managed.sol 43: function _notPartialPaused() internal view { 44: require(!controller.paused(), "Paused"); //@audit: SLOAD 1 45: require(!controller.partialPaused(), "Partial-paused");//@audit: SLOAD 2 46: }
File: /contracts/governance/Governed.sol 53: function acceptOwnership() external { 54: require( 55: pendingGovernor != address(0) && msg.sender == pendingGovernor, 56: "Caller must be pending governor" 57: ); //@audit: SLOAD 1 and SLOAD 2 59: address oldGovernor = governor; 60: address oldPendingGovernor = pendingGovernor; //@audit: SLOAD 3 62: governor = pendingGovernor; //@audit: SLOAD 4 67: }
Caching a mapping's value in a local storage or calldata variable when the value is accessed multiple times saves ~42 gas per access due to not having to perform the same offset calculation every time. Help the Optimizer by saving a storage variable's reference instead of repeatedly fetching it
To help the optimizer,declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array.
As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it
File: /contracts/gateway/L1GraphTokenGateway.sol 152: function addToCallhookWhitelist(address _newWhitelisted) external onlyGovernor { 153: require(_newWhitelisted != address(0), "INVALID_ADDRESS"); 154: require(!callhookWhitelist[_newWhitelisted], "ALREADY_WHITELISTED"); 155: callhookWhitelist[_newWhitelisted] = true; 156: emit AddedToCallhookWhitelist(_newWhitelisted); 157: }
File: /contracts/gateway/L1GraphTokenGateway.sol 164: function removeFromCallhookWhitelist(address _notWhitelisted) external onlyGovernor { 165: require(_notWhitelisted != address(0), "INVALID_ADDRESS"); 166: require(callhookWhitelist[_notWhitelisted], "NOT_WHITELISTED"); 167: callhookWhitelist[_notWhitelisted] = false; 168: emit RemovedFromCallhookWhitelist(_notWhitelisted); 169: }
File: /contracts/l2/token/GraphTokenUpgradeable.sol 74: function permit( 87: keccak256( 88: abi.encode(PERMIT_TYPEHASH, _owner, _spender, _value, nonces[_owner], _deadline) //@audit: nonces[_owner] 97: nonces[_owner] = nonces[_owner].add(1); //@audit: nonces[_owner] 98: _approve(_owner, _spender, _value); 99: }
File: /contracts/governance/Managed.sol 173: function _syncContract(string memory _name) internal { 176: if (addressCache[nameHash] != contractAddress) { 177: addressCache[nameHash] = contractAddress; 178: emit ContractSynced(nameHash, contractAddress); 179: } 180: }
Here, the values emitted shouldn’t be read from storage. The existing memory values should be used instead:
File: /contracts/l2/token/L2GraphToken.sol //@audit: We should emit _gw instead of gateway 59: function setGateway(address _gw) external onlyGovernor { 60: require(_gw != address(0), "INVALID_GATEWAY"); 61: gateway = _gw; 62: emit GatewaySet(gateway); 63: }
File: /contracts/governance/Pausable.sol 26: function _setPartialPaused(bool _toPause) internal { 27: if (_toPause == _partialPaused) { 28: return; 29: } 30: _partialPaused = _toPause; 31: if (_partialPaused) { 32: lastPausePartialTime = block.timestamp; 33: } 34: emit PartialPauseChanged(_partialPaused); 35: }
File: /contracts/governance/Pausable.sol //@audit: We should emit _toPause instead of _paused 40: function _setPaused(bool _toPause) internal { 41: if (_toPause == _paused) { 42: return; 43: } 44: _paused = _toPause; 45: if (_paused) { 46: lastPauseTime = block.timestamp; 47: } 48: emit PauseChanged(_paused); 49: }
File: /contracts/governance/Pausable.sol //@audit: We should emit newPauseGuardian isntead of pauseGuardian 55: function _setPauseGuardian(address newPauseGuardian) internal { 56: address oldPauseGuardian = pauseGuardian; 57: pauseGuardian = newPauseGuardian; 58: emit NewPauseGuardian(oldPauseGuardian, pauseGuardian); 59: }
File: /contracts/governance/Governed.sol //@audit: We should emit _newGovernor instead of pendingGovernor 40: function transferOwnership(address _newGovernor) external onlyGovernor { 44: pendingGovernor = _newGovernor; 46: emit NewPendingOwnership(oldPendingGovernor, pendingGovernor); 47: }
Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot. Finally, if both fields are accessed in the same function, can save ~42 gas per access due to not having to recalculate the key's keccak256 hash (Gkeccak256 - 30 gas) and that calculation's associated stack operations.
File: /contracts/l2/token/GraphTokenUpgradeable.sol 51: mapping(address => bool) private _minters; 52: mapping(address => uint256) public nonces;
Prior to 0.8.10 the compiler inserted extra code, including EXTCODESIZE (100 gas), to check for contract existence for external calls. In more recent solidity versions, the compiler will not insert these checks if the external call has a return value
File: /contracts/gateway/L1GraphTokenGateway.sol 77: IBridge bridge = IInbox(inbox).bridge(); 81: address l2ToL1Sender = IOutbox(bridge.activeOutbox()).l2ToL1Sender(); 273: uint256 escrowBalance = token.balanceOf(escrow);
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"); 164: contractAddress = controller.getContractProxy(_nameHash); 175: address contractAddress = controller.getContractProxy(nameHash);
File: /contracts/upgrades/GraphUpgradeable.sol 24: require(msg.sender == _proxy.admin(), "Caller must be the proxy admin");
File: /contracts/gateway/GraphTokenGateway.sol 20: msg.sender == controller.getGovernor() || msg.sender == pauseGuardian,
Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.
If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.
consequences:
Each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..)
Since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )
See: ethereum/solidity#9232
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: );
Proof: can be tested on remix also, showing results for foundry
pragma solidity ^0.8.6; contract Constant{ bytes32 constant noteDenom = keccak256(bytes("NOTE")); function doConstant() external view returns(bytes32){ return noteDenom; } } contract Immutable{ bytes32 immutable noteDenom = keccak256(bytes("NOTE")); function doImmutable() external view returns(bytes32){ return noteDenom; } }
Results from : forge test --gas-report --optimize
Running 1 test for test/Constant.t.sol:ConstantTest [PASS] testdoThing() (gas: 5416) Test result: ok. 1 passed; 0 failed; finished in 420.08µs Running 1 test for test/Immutable.t.sol:ImmutableTest [PASS] testdoThing() (gas: 5356) Test result: ok. 1 passed; 0 failed; finished in 389.91µs
Instead of using the && operator in a single require statement to check multiple conditions,using multiple require statements with 1 condition per require statement will save 8 GAS per && The gas difference would only be realized if the revert condition is realized(met).
File: /contracts/gateway/L1GraphTokenGateway.sol 142: require(_escrow != address(0) && Address.isContract(_escrow), "INVALID_ESCROW");
File: /contracts/upgrades/GraphProxy.sol 142: require( 143: _pendingImplementation != address(0) && msg.sender == _pendingImplementation, 144: "Caller must be the pending implementation" 145: );
File: /contracts/governance/Governed.sol 54: require( 55: pendingGovernor != address(0) && msg.sender == pendingGovernor, 56: "Caller must be pending governor" 57: );
Proof The following tests were carried out in remix with both optimization turned on and off
function multiple (uint a) public pure returns (uint){ require ( a > 1 && a < 5, "Initialized"); return a + 2; }
Execution cost 21617 with optimization and using && 21976 without optimization and using &&
After splitting the require statement
function multiple(uint a) public pure returns (uint){ require (a > 1 ,"Initialized"); require (a < 5 , "Initialized"); return a + 2; }
Execution cost 21609 with optimization and split require 21968 without optimization and using split require
Up until Solidity 0.8.13: != 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas) != 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)
For uints the minimum value would be 0 and never a negative value. Since it cannot be a negative value, then the check > 0 is essentially checking that the value is not equal to 0 therefore >0 can be replaced with !=0 which saves 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 proofs: https://twitter.com/gzeon/status/1485428085885640706
I suggest changing > 0 with != 0 here:
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");
Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value. I suggest using if(directValue) instead of if(directValue == true) and if(!directValue) instead of if(directValue == false) here:
File: /contracts/gateway/L1GraphTokenGateway.sol //@audit: callhookWhitelist[msg.sender] == true, 213: require( 214: extraData.length == 0 || callhookWhitelist[msg.sender] == true, 215: "CALL_HOOK_DATA_NOT_ALLOWED" 216: );
// 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.
See source Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas), and to avoid Gsset (20000 gas) when changing from ‘false’ to ‘true’, after having been ‘true’ in the past
Instances affected include
File: /contracts/gateway/L1GraphTokenGateway.sol 35: mapping(address => bool) public callhookWhitelist;
File: /contracts/l2/token/GraphTokenUpgradeable.sol 51: mapping(address => bool) private _minters;
File: /contracts/governance/Pausable.sol 8: bool internal _partialPaused; 9: // Paused will pause all major protocol functions 10: bool internal _paused;
File: /contracts/gateway/L1GraphTokenGateway.sol 73: modifier onlyL2Counterpart() { 74: require(inbox != address(0), "INBOX_NOT_SET"); 76: // a message coming from the counterpart gateway was executed by the bridge 77: IBridge bridge = IInbox(inbox).bridge(); 78: require(msg.sender == address(bridge), "NOT_FROM_BRIDGE"); 80: // and the outbox reports that the L2 address of the sender is the counterpart gateway 81: address l2ToL1Sender = IOutbox(bridge.activeOutbox()).l2ToL1Sender(); 82: require(l2ToL1Sender == l2Counterpart, "ONLY_COUNTERPART_GATEWAY"); 83: _; 84: }
The above modifier is only used once on Line 269
Changing abi.encode function to abi.encodePacked can save gas since the abi.encode function pads extra null bytes at the end of the call data, which is unnecessary. Consider using abi.encodePacked() here:
File: /contracts/gateway/L1GraphTokenGateway.sol 249: return abi.encode(seqNum);
File: /contracts/l2/gateway/L2GraphTokenGateway.sol 174: return abi.encode(id);
Every reason string takes at least 32 bytes so make sure your string fits in 32 bytes or it will become more expensive.Each extra chunk of byetes past the original 32 incurs an MSTORE which costs 3 gas
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.
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/governance/Managed.sol 53: require(msg.sender == controller.getGovernor(), "Caller must be Controller governor");
File: /contracts/gateway/GraphTokenGateway.sol 19: require( 20: msg.sender == controller.getGovernor() || msg.sender == pauseGuardian, 21: "Only Governor or Guardian can call" 22: );
I suggest shortening the revert strings to fit in 32 bytes, or using custom errors.
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
Other than the interfaces, the rest of the code seems to be using the following version.
pragma solidity ^0.7.6;
An example of how where we might utilise the custom errors is explained below
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met) 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
Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries). see Source
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");
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/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/GraphProxyStorage.sol 62: require(msg.sender == _admin(), "Caller must be admin");
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/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/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)");
#0 - tmigone
2022-10-21T20:08:49Z
We consider this a high quality submission.