The Graph L2 bridge contest - mcwildy's results

Qa Report

THEGRAPH

Use external visibility modifier for function that are not being invoked by the contract

GraphProxyAdmin.sol:

line#L30:  function getProxyImplementation(IGraphProxy _proxy) public view returns (address) {

line#L43:  function getProxyPendingImplementation(IGraphProxy _proxy) public view returns (address) {

line#L68:  function changeProxyAdmin(IGraphProxy _proxy, address _newAdmin) public onlyGovernor {

line#L77:  function upgrade(IGraphProxy _proxy, address _implementation) public onlyGovernor {

Managed.sol:

line#L18:  * public `syncAllContracts()` function whenever a contract changes in the controller.

Use higher solidity version

Optimizations, bug fixes and additional features come with each solidity patch version update

Managed.sol:

line#L3:   pragma solidity ^0.7.6;

GraphUpgradeable.sol:

line#L3:   pragma solidity ^0.7.6;

IEpochManager.sol:

line#L3:   pragma solidity ^0.7.6;

IController.sol:

line#L3:   pragma solidity >=0.6.12 <0.8.0;

IGraphCurationToken.sol:

line#L3:   pragma solidity ^0.7.6;

L2GraphToken.sol:

line#L3:   pragma solidity ^0.7.6;

GraphProxyStorage.sol:

line#L3:   pragma solidity ^0.7.6;

L1GraphTokenGateway.sol:

line#L3:   pragma solidity ^0.7.6;

L1ArbitrumMessenger.sol:

line#L26:  pragma solidity ^0.7.6;

L2ArbitrumMessenger.sol:

line#L26:  pragma solidity ^0.7.6;

IGraphProxy.sol:

line#L3:   pragma solidity ^0.7.6;

GraphProxy.sol:

line#L3:   pragma solidity ^0.7.6;

GraphTokenUpgradeable.sol:

line#L3:   pragma solidity ^0.7.6;

AddressAliasHelper.sol:

line#L26:  pragma solidity ^0.7.6;

IStakingData.sol:

line#L3:   pragma solidity >=0.6.12 <0.8.0;

ICallhookReceiver.sol:

line#L9:   pragma solidity ^0.7.6;

IStaking.sol:

line#L3:   pragma solidity >=0.6.12 <0.8.0;

Pausable.sol:

line#L3:   pragma solidity ^0.7.6;

L2GraphTokenGateway.sol:

line#L3:   pragma solidity ^0.7.6;

BridgeEscrow.sol:

line#L3:   pragma solidity ^0.7.6;

IRewardsManager.sol:

line#L3:   pragma solidity ^0.7.6;

IGraphToken.sol:

line#L3:   pragma solidity ^0.7.6;

Governed.sol:

line#L3:   pragma solidity ^0.7.6;

GraphProxyAdmin.sol:

line#L3:   pragma solidity ^0.7.6;

GraphTokenGateway.sol:

line#L3:   pragma solidity ^0.7.6;

ICuration.sol:

line#L3:   pragma solidity ^0.7.6;

Unused import statements

ICuration.sol:

line#L5:   import "./IGraphCurationToken.sol";

Missing natspec comments

https://docs.soliditylang.org/en/develop/natspec-format.html

IGraphCurationToken.sol:

line#L1:   // SPDX-License-Identifier: GPL-2.0-or-later

IGraphProxy.sol:

line#L1:   // SPDX-License-Identifier: GPL-2.0-or-later

IGraphToken.sol:

line#L1:   // SPDX-License-Identifier: GPL-2.0-or-later

IEpochManager.sol:

line#L1:   // SPDX-License-Identifier: GPL-2.0-or-later

IController.sol:

line#L1:   // SPDX-License-Identifier: GPL-2.0-or-later

ICuration.sol:

line#L1:   // SPDX-License-Identifier: GPL-2.0-or-later

Events should be emmitted on every critical state changes

Emmiting events is recommended each time when a state variable's value is being changed or just some critical event for the contract has occurred. It also helps off-chain monitoring of the contract's state.

GraphTokenUpgradeable.sol:

line#L150: function _initialize(address _owner, uint256 _initialSupply) internal {

Governed.sol:

line#L31:  function _initialize(address _initGovernor) internal {

Contracts should use a fixed compiler version to avoid potential bugs

L1ArbitrumMessenger.sol:

line#L26:  pragma solidity ^0.7.6;

GraphProxyStorage.sol:

line#L3:   pragma solidity ^0.7.6;

Managed.sol:

line#L3:   pragma solidity ^0.7.6;

GraphProxyAdmin.sol:

line#L3:   pragma solidity ^0.7.6;

BridgeEscrow.sol:

line#L3:   pragma solidity ^0.7.6;

ICallhookReceiver.sol:

line#L9:   pragma solidity ^0.7.6;

L2GraphToken.sol:

line#L3:   pragma solidity ^0.7.6;

Pausable.sol:

line#L3:   pragma solidity ^0.7.6;

GraphUpgradeable.sol:

line#L3:   pragma solidity ^0.7.6;

L1GraphTokenGateway.sol:

line#L3:   pragma solidity ^0.7.6;

Governed.sol:

line#L3:   pragma solidity ^0.7.6;

GraphTokenGateway.sol:

line#L3:   pragma solidity ^0.7.6;

L2ArbitrumMessenger.sol:

line#L26:  pragma solidity ^0.7.6;

GraphTokenUpgradeable.sol:

line#L3:   pragma solidity ^0.7.6;

L2GraphTokenGateway.sol:

line#L3:   pragma solidity ^0.7.6;

GraphProxy.sol:

line#L3:   pragma solidity ^0.7.6;

GAS OPTIMIZATIONS REPORT

THEGRAPH

Use 1 and 2 instead of true and false

EVM word size is 32 bytes. The uint256 1 and uint256 2 will be cheaper to use than the boolean true and boolean false since the first are 32 bytes long and the last are only 1 byte

Pausable.sol:

line#L8:   bool internal _partialPaused;

line#L10:  bool internal _paused;

Function inlining

If a function is called only once it can be inlined in order to save 20 gas

GraphTokenUpgradeable.sol:

line#L195: function _getChainID() private pure returns (uint256) {

L1GraphTokenGateway.sol:

line#L290: function parseOutboundData(bytes memory _data)

L2GraphTokenGateway.sol:

line#L286: function parseOutboundData(bytes memory _data) private view returns (address, bytes memory) {

Pack storage variables in less slots to save gas

Pausable.sol:

line#L8:   /// @audit Currently storage variables are packed in 4 slots but could fit in 3

Mark functions as payable to avoid the low-level evm is-a-payment-provided check

The EVM checks whether a payment is provided to the function and this check costs additional gas. If the function is marked as payable there is no such check

Managed.sol:

line#L52:  function _onlyGovernor() internal view {

line#L56:  function _onlyController() internal view {

Governed.sol:

line#L40:  function transferOwnership(address _newGovernor) external onlyGovernor {

GraphUpgradeable.sol:

line#L50:  function acceptProxy(IGraphProxy _proxy) external onlyProxyAdmin(_proxy) {

L2GraphToken.sol:

line#L80:  function bridgeMint(address _account, uint256 _amount) external override onlyGateway {

line#L90:  function bridgeBurn(address _account, uint256 _amount) external override onlyGateway {

Use constant and immutable keywords for storage varibles if they doesn't change through contract's lifecycle

That way the variable will go to the contract's bytecode and will not allocate a storage slot

Managed.sol:

line#L29:  uint256[10] private __gap;

Use calldata where possible instead of memory to save gas

L1GraphTokenGateway.sol:

line#L290: function parseOutboundData(bytes memory _data)

line#L331: bytes memory _data

L2GraphTokenGateway.sol:

line#L266: bytes memory _data

           /// @audit Store `_data` in calldata.
line#L286: function parseOutboundData(bytes memory _data) private view returns (address, bytes memory) {

L2ArbitrumMessenger.sol:

line#L41:  bytes memory _data

Managed.sol:

line#L173: function _syncContract(string memory _name) internal {

L1ArbitrumMessenger.sol:

line#L48:  L2GasParams memory _l2GasParams,

line#L49:  bytes memory _data

line#L75:  bytes memory _data

x < y + 1 is cheaper than x <= y

L1GraphTokenGateway.sol:

line#L224: require(msg.value >= expectedEth, "WRONG_ETH_VALUE");

line#L275: require(_amount <= escrowBalance, "BRIDGE_OUT_OF_FUNDS");

GraphTokenUpgradeable.sol:

line#L95:  require(_deadline == 0 || block.timestamp <= _deadline, "GRT: expired permit");

Use uint256 instead of the smaller uints where possible

The word-size in ethereum is 32 bytes which means that every variables which is sized with less bytes will cost more to operate with

GraphTokenUpgradeable.sol:

line#L79:  uint8 _v,

IGraphToken.sol:

line#L33:  uint8 _v,

IStaking.sol:

line#L32:  function setThawingPeriod(uint32 _thawingPeriod) external;

line#L34:  function setCurationPercentage(uint32 _percentage) external;

line#L38:  function setChannelDisputeEpochs(uint32 _channelDisputeEpochs) external;

line#L40:  function setMaxAllocationEpochs(uint32 _maxAllocationEpochs) external;

line#L44:  function setDelegationRatio(uint32 _delegationRatio) external;

line#L47:  uint32 _indexingRewardCut,

line#L49:  uint32 _cooldownBlocks

line#L52:  function setDelegationParametersCooldown(uint32 _blocks) external;

line#L56:  function setDelegationTaxPercentage(uint32 _percentage) external;

IStakingData.sol:

line#L37:  uint32 cooldownBlocks;

line#L38:  uint32 indexingRewardCut;

line#L39:  uint32 queryFeeCut;

AddressAliasHelper.sol:

line#L29:  uint160 constant offset = uint160(0x1111000000000000000000000000000000001111);

ICuration.sol:

line#L10:  function setDefaultReserveRatio(uint32 _defaultReserveRatio) external;

line#L14:  function setCurationTaxPercentage(uint32 _percentage) external;

Use custom errors instead of require strings

Governed.sol:

line#L24:  require(msg.sender == governor, "Only Governor can call");

line#L41:  require(_newGovernor != address(0), "Governor must be set");

line#L54:  require(
line#L55:  pendingGovernor != address(0) && msg.sender == pendingGovernor,
line#L56:  "Caller must be pending governor"
line#L57:  );

GraphTokenUpgradeable.sol:

line#L60:  require(isMinter(msg.sender), "Only minter can call");

line#L94:  require(_owner == recoveredAddress, "GRT: invalid permit");

line#L106: require(_account != address(0), "INVALID_MINTER");

line#L115: require(_minters[_account], "NOT_A_MINTER");

GraphUpgradeable.sol:

line#L24:  require(msg.sender == _proxy.admin(), "Caller must be the proxy admin");

line#L32:  require(msg.sender == _implementation(), "Caller must be the implementation");

GraphProxyStorage.sol:

line#L62:  require(msg.sender == _admin(), "Caller must be admin");

L1ArbitrumMessenger.sol:

line#L100: require(l2ToL1Sender != address(0), "NO_SENDER");

L2GraphToken.sol:

line#L36:  require(msg.sender == gateway, "NOT_GATEWAY");

line#L49:  require(_owner != address(0), "Owner must be set");

line#L60:  require(_gw != address(0), "INVALID_GATEWAY");

line#L70:  require(_addr != address(0), "INVALID_L1_ADDRESS");

L2GraphTokenGateway.sol:

line#L69:  require(
line#L70:  msg.sender == AddressAliasHelper.applyL1ToL2Alias(l1Counterpart),
line#L71:  "ONLY_COUNTERPART_GATEWAY"
line#L72:  );

line#L98:  require(_l2Router != address(0), "INVALID_L2_ROUTER");

line#L118: require(_l1Counterpart != address(0), "INVALID_L1_COUNTERPART");

line#L145: require(_l1Token == l1GRT, "TOKEN_NOT_GRT");

line#L147: require(msg.value == 0, "INVALID_NONZERO_VALUE");

line#L148: require(_to != address(0), "INVALID_DESTINATION");

line#L233: require(_l1Token == l1GRT, "TOKEN_NOT_GRT");

line#L234: require(msg.value == 0, "INVALID_NONZERO_VALUE");

Managed.sol:

line#L44:  require(!controller.paused(), "Paused");

line#L45:  require(!controller.partialPaused(), "Partial-paused");

line#L53:  require(msg.sender == controller.getGovernor(), "Caller must be Controller governor");

line#L57:  require(msg.sender == address(controller), "Caller must be Controller");

GraphProxy.sol:

line#L105: require(_newAdmin != address(0), "Cannot change the admin of a proxy to the zero address");

line#L141: require(Address.isContract(_pendingImplementation), "Implementation must be a contract");

line#L142: require(
line#L143: _pendingImplementation != address(0) && msg.sender == _pendingImplementation,
line#L144: "Caller must be the pending implementation"
line#L145: );

line#L157: require(msg.sender != _admin(), "Cannot fallback to proxy target");

GraphTokenGateway.sol:

line#L19:  require(
line#L20:  msg.sender == controller.getGovernor() || msg.sender == pauseGuardian,
line#L21:  "Only Governor or Guardian can call"
line#L22:  );

line#L31:  require(_newPauseGuardian != address(0), "PauseGuardian must be set");

line#L40:  require(!_paused, "Paused (contract)");

L1GraphTokenGateway.sol:

line#L74:  require(inbox != address(0), "INBOX_NOT_SET");

line#L78:  require(msg.sender == address(bridge), "NOT_FROM_BRIDGE");

line#L110: require(_inbox != address(0), "INVALID_INBOX");

line#L111: require(_l1Router != address(0), "INVALID_L1_ROUTER");

line#L132: require(_l2Counterpart != address(0), "INVALID_L2_COUNTERPART");

line#L142: require(_escrow != address(0) && Address.isContract(_escrow), "INVALID_ESCROW");

line#L154: require(!callhookWhitelist[_newWhitelisted], "ALREADY_WHITELISTED");

line#L165: require(_notWhitelisted != address(0), "INVALID_ADDRESS");

line#L200: require(_l1Token == address(token), "TOKEN_NOT_GRT");

line#L201: require(_amount > 0, "INVALID_ZERO_AMOUNT");

line#L213: require(
line#L214: extraData.length == 0 || callhookWhitelist[msg.sender] == true,
line#L215: "CALL_HOOK_DATA_NOT_ALLOWED"
line#L216: );

line#L217: require(maxSubmissionCost > 0, "NO_SUBMISSION_COST");

line#L271: require(_l1Token == address(token), "TOKEN_NOT_GRT");

line#L275: require(_amount <= escrowBalance, "BRIDGE_OUT_OF_FUNDS");

Use if (x != 0) instead of if (x > 0) for uints comparison

Saves 3 gas per if-check

L2GraphTokenGateway.sol:

line#L146: require(_amount > 0, "INVALID_ZERO_AMOUNT");

L1GraphTokenGateway.sol:

line#L201: require(_amount > 0, "INVALID_ZERO_AMOUNT");

line#L217: require(maxSubmissionCost > 0, "NO_SUBMISSION_COST");

Do not use strings longer than 32 bytes

GraphProxy.sol:

line#L105: require(_newAdmin != address(0), "Cannot change the admin of a proxy to the zero address");

line#L141: require(Address.isContract(_pendingImplementation), "Implementation must be a contract");

line#L142: require(
line#L143: _pendingImplementation != address(0) && msg.sender == _pendingImplementation,
line#L144: "Caller must be the pending implementation"
line#L145: );

GraphTokenGateway.sol:

line#L19:  require(
line#L20:  msg.sender == controller.getGovernor() || msg.sender == pauseGuardian,
line#L21:  "Only Governor or Guardian can call"
line#L22:  );

Managed.sol:

line#L53:  require(msg.sender == controller.getGovernor(), "Caller must be Controller governor");

GraphUpgradeable.sol:

line#L32:  require(msg.sender == _implementation(), "Caller must be the implementation");

Avoid using && in require() or if() statements

Use multiple require()/if statements instead

GraphProxy.sol:

line#L142: require(
line#L143: _pendingImplementation != address(0) && msg.sender == _pendingImplementation,
line#L144: "Caller must be the pending implementation"
line#L145: );

L1GraphTokenGateway.sol:

line#L142: require(_escrow != address(0) && Address.isContract(_escrow), "INVALID_ESCROW");

Governed.sol:

line#L54:  require(
line#L55:  pendingGovernor != address(0) && msg.sender == pendingGovernor,
line#L56:  "Caller must be pending governor"
line#L57:  );

Cache storage variables in memory

Optimization comes from using MSTORE and MLOAD instead of SSTORE and SLOAD

Governed.sol:

           /// @audit Cache `pendingGovernor`. Used 4 times in `acceptOwnership`
line#L55:  pendingGovernor != address(0) && msg.sender == pendingGovernor,
line#L60:  address oldPendingGovernor = pendingGovernor;
line#L62:  governor = pendingGovernor;
line#L66:  emit NewPendingOwnership(oldPendingGovernor, pendingGovernor);

Pausable.sol:

           /// @audit Cache `_partialPaused`. Used 3 times in `_setPartialPaused`
line#L27:  if (_toPause == _partialPaused) {
line#L31:  if (_partialPaused) {
line#L34:  emit PartialPauseChanged(_partialPaused);

           /// @audit Cache `_paused`. Used 3 times in `_setPaused`
line#L41:  if (_toPause == _paused) {
line#L45:  if (_paused) {
line#L48:  emit PauseChanged(_paused);

If a constant variable initialization includes keccak256(), sha256(), etc. it should be marked as immutable instead

The result of the calculation of the hash (or whatever the expression is) is expected to be saved once compiled, but that's not the case. If marked as constant the calculation will be executed every time the variable is accessed.

GraphTokenUpgradeable.sol:

line#L38:  bytes32 private constant DOMAIN_NAME_HASH = keccak256("Graph Token");

line#L39:  bytes32 private constant DOMAIN_VERSION_HASH = keccak256("0");

Do not compare a boolean to true or false

Directly use the boolean

L1GraphTokenGateway.sol:

line#L214: extraData.length == 0 || callhookWhitelist[msg.sender] == true,