Platform: Code4rena
Start Date: 28/10/2022
Pot Size: $165,500 USDC
Total HM: 2
Participants: 24
Period: 12 days
Judge: GalloDaSballo
Total Solo HM: 1
Id: 177
League: ETH
Rank: 12/24
Findings: 2
Award: $480.53
🌟 Selected for report: 0
🚀 Solo Findings: 0
250.7706 USDC - $250.77
| Issue | Contexts | |
|---|---|---|
| LOW‑1 | Missing Checks for Address(0x0) | 5 |
| LOW‑2 | Missing Contract-existence Checks Before Low-level Calls | 1 |
| LOW‑3 | Contracts are not using their OZ Upgradeable counterparts | 8 |
| LOW‑4 | Missing parameter validation in constructor | 3 |
Total: 17 contexts over 4 issues
| Issue | Contexts | |
|---|---|---|
| NC‑1 | Critical Changes Should Use Two-step Procedure | 10 |
| NC‑2 | Use a more recent version of Solidity | All in-scope contracts |
| NC‑3 | Redundant Cast | 1 |
| NC‑4 | Event Is Missing Indexed Fields | 5 |
| NC‑5 | require() / revert() Statements Should Have Descriptive Reason Strings | 12 |
| NC‑6 | Implementation contract may not be initialized | 8 |
| NC‑7 | Avoid Floating Pragmas: The Version Should Be Locked | All in-scope contracts |
| NC‑8 | Non-usage of specific imports | 77 |
| NC‑9 | Expressions for constant values such as a call to keccak256(), should use immutable rather than constant | 2 |
| NC‑10 | Lines are too long | 8 |
| NC‑11 | Open TODOs | 5 |
Total: 206 contexts over 11 issues
Lack of zero-address validation on address parameters may lead to transaction reverts, waste gas, require resubmission of transactions and may even force contract redeployments in certain cases within the protocol.
73: function initialize: address _l2TokenFactory
113: function deposit: address _l1Token
180: function claimFailedDeposit: address _depositSender
133: function claimFailedDeposit: address _depositSender
102: function bridgeMint: address _to
Consider adding explicit zero-address validation on input parameters of address type.
Low-level calls return success if there is no code present at the specified address.
282: (bool success, bytes memory data) = _init.delegatecall(_calldata);
Add a check to verify that _init.code.length > 0
The non-upgradeable standard version of OpenZeppelin’s library are inherited / used by the contracts. It would be safer to use the upgradeable versions of the library contracts to avoid unexpected behaviour.
5: import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol"; 6: import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
5: import "@openzeppelin/contracts/proxy/beacon/BeaconProxy.sol"; 6: import "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol";
Where applicable, use the contracts from @openzeppelin/contracts-upgradeable instead of @openzeppelin/contracts.
constructorSome parameters of constructors are not checked for invalid values.
32: address _l1Bridge 34: address _governor
30: address _l1Bridge
https://github.com/code-423n4/2022-10-zksync/tree/main/zksync/contracts/bridge/L2ETHBridge.sol#L30
Validate the parameters.
The critical procedures should be two step process.
See similar findings in previous Code4rena contests for reference: https://code4rena.com/reports/2022-06-illuminate/#2-critical-changes-should-use-two-step-procedure
57: function setPublicAccess(address _target, bool _enable) external onlyOwner {
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/common/AllowList.sol#L57
65: function setBatchPublicAccess(address[] calldata _targets, bool[] calldata _enables) external onlyOwner {
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/common/AllowList.sol#L65
89: function setBatchPermissionToCall(
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/common/AllowList.sol#L89
113: function setPermissionToCall(
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/common/AllowList.sol#L113
45: function setValidator(address _validator, bool _active) external onlyGovernor {
54: function setL2BootloaderBytecodeHash(bytes32 _l2BootloaderBytecodeHash) external onlyGovernor {
69: function setL2DefaultAccountBytecodeHash(bytes32 _l2DefaultAccountBytecodeHash) external onlyGovernor {
84: function setPorterAvailability(bool _zkPorterIsAvailable) external onlyGovernor {
94: function setVerifier(Verifier _newVerifier) external onlyGovernor {
104: function setVerifierParams(VerifierParams calldata _newVerifierParams) external onlyGovernor {
Lack of two-step procedure for critical operations leaves them error-prone. Consider adding two step procedure on the critical functions.
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>) Use a solidity version of at least 0.8.13 to get the ability to use using for with a list of free functions
Relevant to all in-scope contracts.
Consider updating to a more recent solidity version.
The type of the variable is the same as the type to which the variable is being cast
206: uint256(_txId)
Index 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.
event DiamondCutProposal(Diamond.FacetCut[] _facetCuts, address _initAddress);
event BlocksRevert(uint256 totalBlocksCommitted, uint256 totalBlocksVerified, uint256 totalBlocksExecuted);
event NewVerifierParams(VerifierParams oldVerifierParams, VerifierParams newVerifierParams);
event NewPriorityRequest( uint256 txId, bytes32 txHash, uint64 expirationBlock, L2CanonicalTransaction transaction, bytes[] factoryDeps );
event DiamondCut(FacetCut[] facetCuts, address initAddress, bytes initCalldata);
require() / revert() Statements Should Have Descriptive Reason Strings145: require(amount != 0);
221: require(_message.length == 56);
224: require(bytes4(functionSignature) == this.finalizeWithdrawal.selector);
238: require(callSuccess);
43: require(expectedNumberOfLayer1Txs == _newBlock.numberOfLayer1Txs);
45: require(l2BlockTimestamp == _newBlock.timestamp);
297: require(_recurisiveAggregationInput.length == 4);
50: require(_l1Token == CONVENTIONAL_ETH_ADDRESS);
https://github.com/code-423n4/2022-10-zksync/tree/main/zksync/contracts/bridge/L2ETHBridge.sol#L50
96: require(msg.sender == l2Bridge);
116: revert();
122: revert();
128: revert();
OpenZeppelin recommends that the initializer modifier be applied to constructors. Per OZs Post implementation contract should be initialized to avoid potential griefs or exploits. https://forum.openzeppelin.com/t/uupsupgradeable-vulnerability-post-mortem/15680/5
57: constructor(IMailbox _mailbox, IAllowList _allowList) reentrancyGuardInitializer { 58: zkSyncMailbox = _mailbox; 59: allowList = _allowList; 60: }
48: constructor(IMailbox _mailbox, IAllowList _allowList) reentrancyGuardInitializer { 49: zkSyncMailbox = _mailbox; 50: allowList = _allowList; 51: }
32: constructor(address _owner) { 33: require(_owner != address(0), "kq"); 34: owner = _owner; 35: }
10: constructor(uint256 _chainId, Diamond.DiamondCutData memory _diamondCut) { 13: require(_chainId == block.chainid, "pr"); 14: Diamond.diamondCut(_diamondCut); 15: }
13: constructor() { 16: assert(SECURITY_COUNCIL_APPROVALS_FOR_EMERGENCY_UPGRADE > 0); 17: }
constructor( address _l1Bridge, bytes32 _l2TokenProxyBytecodeHash, address _governor ) { l1Bridge = _l1Bridge; l2TokenProxyBytecodeHash = _l2TokenProxyBytecodeHash; address l2StandardToken = address(new L2StandardERC20{salt: bytes32(0)}()); l2TokenFactory = new UpgradeableBeacon{salt: bytes32(0)}(l2StandardToken); l2TokenFactory.transferOwnership(_governor); }
constructor(address _l1Bridge) { l1Bridge = _l1Bridge; IL2EthInitializable(address(ETH_TOKEN_SYSTEM_CONTRACT_ADDRESS)).initialization(address(this)); }
Avoid floating pragmas for non-library contracts.
While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.
A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.
It is recommended to pin to a concrete compiler version.
Found usage of floating pragmas ^0.8.0 of Solidity in all in-scope contracts.
The current form of relative path import is not recommended for use because it can unpredictably pollute the namespace. Instead, the Solidity docs recommend specifying imported symbols explicitly. https://docs.soliditylang.org/en/v0.8.15/layout-of-source-files.html#importing-other-source-files
8: import "./interfaces/IL1Bridge.sol"; 9: import "./interfaces/IL2Bridge.sol"; 11: import "../common/interfaces/IAllowList.sol"; 12: import "../common/AllowListed.sol"; 13: import "../common/libraries/UnsafeBytes.sol"; 14: import "../common/ReentrancyGuard.sol"; 15: import "../common/L2ContractHelper.sol";
5: import "./interfaces/IL1Bridge.sol"; 6: import "./interfaces/IL2Bridge.sol"; 8: import "../common/interfaces/IAllowList.sol"; 9: import "../common/AllowListed.sol"; 10: import "../common/libraries/UnsafeBytes.sol"; 11: import "../common/L2ContractHelper.sol"; 12: import "../common/ReentrancyGuard.sol";
5: import "./interfaces/IAllowList.sol"; 6: import "./libraries/UncheckedMath.sol";
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/common/AllowList.sol#L5-L6
5: import "./interfaces/IAllowList.sol";
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/common/AllowListed.sol#L5
5: import "../common/interfaces/IAllowList.sol"; 6: import "./interfaces/IExecutor.sol"; 7: import "./libraries/Diamond.sol"; 8: import "./facets/Base.sol"; 9: import "./Config.sol";
5: import "./libraries/Diamond.sol";
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/zksync/DiamondProxy.sol#L5
5: import "./Verifier.sol"; 6: import "../common/interfaces/IAllowList.sol"; 7: import "./libraries/PriorityQueue.sol";
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/zksync/Storage.sol#L5-L7
5: import "../Storage.sol"; 6: import "../../common/ReentrancyGuard.sol"; 7: import "../../common/AllowListed.sol";
5: import "../interfaces/IDiamondCut.sol"; 6: import "../libraries/Diamond.sol"; 7: import "../Config.sol"; 8: import "./Base.sol";
5: import "./Base.sol"; 6: import "../Config.sol"; 7: import "../interfaces/IExecutor.sol"; 8: import "../libraries/PairingsBn254.sol"; 9: import "../libraries/PriorityQueue.sol"; 10: import "../../common/libraries/UncheckedMath.sol"; 11: import "../../common/libraries/UnsafeBytes.sol"; 12: import "../../common/L2ContractHelper.sol";
5: import "./Base.sol"; 6: import "../libraries/Diamond.sol"; 7: import "../libraries/PriorityQueue.sol"; 8: import "../../common/libraries/UncheckedMath.sol"; 9: import "../interfaces/IGetters.sol";
5: import "../interfaces/IGovernance.sol"; 6: import "../../common/L2ContractHelper.sol"; 7: import "./Base.sol";
5: import "../interfaces/IMailbox.sol"; 6: import "../libraries/Merkle.sol"; 7: import "../libraries/PriorityQueue.sol"; 8: import "../Storage.sol"; 9: import "../Config.sol"; 10: import "../../common/libraries/UncheckedMath.sol"; 11: import "../../common/L2ContractHelper.sol"; 12: import "./Base.sol";
5: import "../libraries/Diamond.sol";
5: import "../libraries/PriorityQueue.sol";
5: import "../Verifier.sol"; 6: import "../Storage.sol";
5: import "./IMailbox.sol"; 6: import "./IGovernance.sol"; 7: import "./IExecutor.sol"; 8: import "./IDiamondCut.sol"; 9: import "./IGetters.sol";
5: import "../../common/libraries/UncheckedMath.sol";
8: import "./interfaces/IL1Bridge.sol"; 9: import "./interfaces/IL2Bridge.sol"; 10: import "./interfaces/IL2StandardToken.sol"; 12: import "./L2StandardERC20.sol";
7: import "./interfaces/IL1Bridge.sol"; 8: import "./interfaces/IL2Bridge.sol"; 9: import "./interfaces/IL2EthInitializable.sol"; 10: import "./interfaces/IL2StandardToken.sol";
6: import "./interfaces/IL2StandardToken.sol"; 7: import "../ExternalDecoder.sol";
Use specific imports syntax per solidity docs recommendation.
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.
41: bytes32 constant CREATE2_PREFIX = keccak256("zksyncCreate2");
24: bytes32 constant CREATE2_PREFIX = keccak256("zksyncCreate2");
https://github.com/code-423n4/2022-10-zksync/tree/main/zksync/contracts/L2ContractHelper.sol#L24
Usually lines in source code are limited to 80 characters. Today's screens are much larger so it's reasonable to stretch this in some cases. Since the files will most likely reside in GitHub, and GitHub starts using a scroll bar in all cases when the length is over 164 characters, the lines below should be split when they reach that length Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length
59: /// @dev NOTE: we must reserve for users enough time to send full exit operation, wait maximum time for processing this operation and withdraw funds from it.
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/zksync/Config.sol#L59
104: /// @param _diamondCutHash The hash of the diamond cut that security council members want to approve. Needed to prevent unintentional approvals, including reorg attacks
9: /// @param indexRepeatedStorageChanges The serial number of the shortcut index that's used as a unique identifier for storage keys that were used twice or more
29: /// @param indexRepeatedStorageChanges The serial number of the shortcut index that's used as a unique identifier for storage keys that were used twice or more
14: /// @param ergsPerPubdataByteLimit Maximum number of ergs that will cost one byte of pubdata (every piece of data that will be stored on L1 as calldata)
15: /// @param maxFeePerErg The absolute maximum sender willing to pay per unit of ergs to get the transaction included in a block. Analog to the EIP-1559 `maxFeePerGas` on an L1 transactions
16: /// @param maxPriorityFeePerErg The additional fee that is paid directly to the validator to incentivize them to include the transaction in a block. Analog to the EIP-1559 `maxPriorityFeePerGas` on an L1 transactions
17: /// @param paymaster The address of the EIP-4337 paymaster, that will pay fees for the transaction. `uint256` type for possible address format changes and maintaining backward compatibility
An open TODO is present. It is recommended to avoid open TODOs as they may indicate programming errors that still need to be fixed.
28: // TODO: change constant to the real root hash of empty Merkle tree (SMA-184)
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/zksync/Config.sol#L28
94: // TODO: estimate gas for L1 execute
127: // TODO: Restore after stable priority op fee modeling. (SMA-1230)
169: layer2Tip: uint192(0) // TODO: Restore after fee modeling will be stable. (SMA-1230)
56: /// TODO: The verifier integration is not finished yet, change the structure for compatibility later
#0 - miladpiri
2022-11-23T14:32:01Z
IMO, should be considered as high quality report!
#1 - GalloDaSballo
2022-11-24T01:55:44Z
LOW‑1 | Missing Checks for Address(0x0) | 5 L
LOW‑2 | Missing Contract-existence Checks Before Low-level Calls | 1 Incorrect, return value is checked, meaning the contract must exist
LOW‑3 | Contracts are not using their OZ Upgradeable counterparts | 8 R
LOW‑4 | Missing parameter validation in constructor | 3 Already awarded
NC‑1 | Critical Changes Should Use Two-step Procedure | 10 NC
NC‑2 | Use a more recent version of Solidity | All in-scope contracts NC
NC‑3 | Redundant Cast | 1 NC
NC‑4 | Event Is Missing Indexed Fields | 5 Disputed in lack of why
NC‑5 | require() / revert() Statements Should Have Descriptive Reason Strings | 12 NC
NC‑6 | Implementation contract may not be initialized | 8 Invalid, they have the initializer
NC‑7 | Avoid Floating Pragmas: The Version Should Be Locked | All in-scope contracts NC
NC‑8 | Non-usage of specific imports | 77 NC
NC‑9 | Expressions for constant values such as a call to keccak256(), should use immutable rather than constant | 2 invalid
NC‑10 | Lines are too long | 8 NC
NC‑11 | Open TODOs NC
#2 - GalloDaSballo
2022-12-03T00:41:27Z
1L 1R 8NC
#3 - c4-judge
2022-12-03T19:09:13Z
GalloDaSballo marked the issue as grade-b
🌟 Selected for report: 0x1f8b
Also found by: Aymen0909, HardlyCodeMan, IllIllI, ReyAdmirado, Rolezn, TomJ, c3phas, gogo, mcwildy
229.7602 USDC - $229.76
| Issue | Contexts | |
|---|---|---|
| GAS‑1 | ++i/i++ Should Be unchecked{++i}/unchecked{i++} When It Is Not Possible For Them To Overflow, As Is The Case When Used In For- And While-loops | 4 |
| GAS‑2 | Splitting require() Statements That Use && Saves Gas | 2 |
| GAS‑3 | abi.encode() is less efficient than abi.encodepacked() | 17 |
| GAS‑4 | Use calldata instead of memory for function parameters | 3 |
| GAS‑5 | Public Functions To External | 2 |
| GAS‑6 | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 11 |
| GAS‑7 | Optimize names to save gas | All in-scope contracts |
| GAS‑8 | Use uint256(1)/uint256(2) instead for true and false boolean states | 16 |
| GAS‑9 | Structs can be packed into fewer storage slots | 2 |
| GAS‑10 | Superfluous event fields | 2 |
| GAS‑11 | Do not calculate constants | 3 |
Total: 80 contexts over 11 issues
++i/i++ Should Be unchecked{++i}/unchecked{i++} When It Is Not Possible For Them To Overflow, As Is The Case When Used In For- And While-loopsThe unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas PER LOOP
94: for (uint256 i = 0; i < facetCutsLength; ++i) {
132: for (uint256 i = 0; i < selectorsLength; ++i) {
153: for (uint256 i = 0; i < selectorsLength; ++i) {
173: for (uint256 i = 0; i < selectorsLength; ++i) {
require() statements that use && saves gasInstead of using operator && on a single require. Using a two require can save more gas.
i.e.
for require(version == 1 && _bytecodeHash[1] == bytes1(0), "zf"); use:
require(version == 1); require(_bytecodeHash[1] == bytes1(0));
65: require(version == 1 && _bytecodeHash[1] == bytes1(0), "zf");
65: require(version == 1 && _bytecodeHash[1] == bytes1(0), "zf");
abi.encode() is less efficient than abi.encodepacked()See for more information: https://github.com/ConnorBlockchain/Solidity-Encode-Gas-Comparison
82: bytes memory create2Input = abi.encode(address(this), l2ProxyTokenBytecodeHash, _governor);
168: data = abi.encode(data1, data2, data3);
283: bytes32 constructorInputHash = keccak256(abi.encode(address(l2TokenFactory), ""));
58: bytes memory create2Input = abi.encode(address(this));
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/bridge/L1EthBridge.sol#L58
54: s.storedBlockHashes[0] = keccak256(abi.encode(storedBlockZero));
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/zksync/DiamondInit.sol#L54
27: s.diamondCutStorage.proposedDiamondCutHash = keccak256(abi.encode(_facetCuts, _initAddress));
61: keccak256(abi.encode(_diamondCut.facetCuts, _diamondCut.initAddress)),
85: return keccak256(abi.encode(_previousBlock.blockHash, _newBlock.newStateRoot));
122: chainedPriorityTxsHash = keccak256(abi.encode(chainedPriorityTxsHash, canonicalTxHash));
182: concatHash = keccak256(abi.encode(concatHash, priorityOp.canonicalTxHash));
359: return keccak256(abi.encode(passThroughDataHash, metadataHash, auxiliaryOutputHash));
381: return abi.encode(_block.l2LogsTreeRoot, l2ToL1LogsHash, initialStorageChangesHash, repeatedStorageChangesHash);
386: return keccak256(abi.encode(_storedBlockInfo));
163: canonicalTxHash = keccak256(abi.encode(transaction));
30: currentHash = keccak256(abi.encode(currentHash, _path[i]));
32: currentHash = keccak256(abi.encode(_path[i], currentHash));
114: bytes32 constructorInputHash = keccak256(abi.encode(address(l2TokenFactory), ""));
In some cases, having function arguments in calldata instead of memory is more optimal.
Consider the following generic example:
contract C { function add(uint[] memory arr) external returns (uint sum) { uint length = arr.length; for (uint i = 0; i < arr.length; i++) { sum += arr[i]; } } }
In the above example, the dynamic array arr has the storage location memory. When the function gets called externally, the array values are kept in calldata and copied to memory during ABI decoding (using the opcode calldataload and mstore). And during the for loop, arr[i] accesses the value in memory using a mload. However, for the above example this is inefficient. Consider the following snippet instead:
contract C { function add(uint[] calldata arr) external returns (uint sum) { uint length = arr.length; for (uint i = 0; i < arr.length; i++) { sum += arr[i]; } } }
In the above snippet, instead of going via memory, the value is directly read from calldata using calldataload. That is, there are no intermediate memory operations that carries this value.
Gas savings: In the former example, the ABI decoding begins with copying value from calldata to memory in a for loop. Each iteration would cost at least 60 gas. In the latter example, this can be completely avoided. This will also reduce the number of instructions and therefore reduces the deploy time cost of the contract.
In short, use calldata instead of memory if the function argument is only read.
Note that in older Solidity versions, changing some function arguments from memory to calldata may cause "unimplemented feature error". This can be avoided by using a newer (0.8.*) Solidity compiler.
Examples Note: The following pattern is prevalent in the codebase:
function f(bytes memory data) external { (...) = abi.decode(data, (..., types, ...)); }
Here, changing to bytes calldata will decrease the gas. The total savings for this change across all such uses would be quite significant.
function _addFunctions( address _facet, bytes4[] memory _selectors, bool _isFacetFreezable ) private {
function _replaceFunctions( address _facet, bytes4[] memory _selectors, bool _isFacetFreezable ) private {
function _removeFunctions(address _facet, bytes4[] memory _selectors) private {
The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.
function l2TransactionBaseCost( uint256, uint256, uint32 ) public pure returns (uint256) {
function l2TokenAddress(address _l1Token) public view override returns (address) {
uints/ints smaller than 32 bytes (256 bits) incurs overheadWhen 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
Each operation involving a uint8 costs an extra 22-28 gas (depending on whether the other operand is also a variable of type uint8) as compared to ones involving uint256, due to the compiler having to clear the higher bits of the memory word before operating on the uint8, as well as the associated stack operations of doing so. Use a larger size then downcast where needed
26: uint160 constant SYSTEM_CONTRACTS_OFFSET = 0x8000;
64: uint8 version = uint8(_bytecodeHash[0]);
40: uint8 l2ShardId;
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/zksync/Storage.sol#L40
54: uint16 txNumberInBlock;
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/zksync/Storage.sol#L54
125: uint64 expirationBlock = uint64(block.number + PRIORITY_EXPIRATION);
41: uint64 blockNumber; 42: uint64 timestamp; 43: uint64 indexRepeatedStorageChanges;
24: uint16 selectorPosition;
33: uint16 facetPosition;
207: uint16 selectorPosition = uint16(ds.facetToSelectors[_facet].selectors.length);
12: uint64 expirationBlock; 13: uint192 layer2Tip;
17: uint160 constant SYSTEM_CONTRACTS_OFFSET = 0x8000;
https://github.com/code-423n4/2022-10-zksync/tree/main/zksync/contracts/L2ContractHelper.sol#L17
29: uint8 private decimals_;
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. 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
Relevant to all in-scope contracts.
uint256(1)/uint256(2) instead for true and false boolean statesIf you don't use boolean for storage you will avoid Gwarmaccess 100 gas. In addition, state changes of boolean from true to false can cost up to ~20000 gas rather than uint256(2) to uint256(1) that would cost significantly less.
40: mapping(uint256 => mapping(uint256 => bool)) public isWithdrawalFinalized;
252: isWithdrawalFinalized[_l2BlockNumber][_l2MessageIndex] = true;
37: mapping(uint256 => mapping(uint256 => bool)) public isWithdrawalFinalized;
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/bridge/L1EthBridge.sol#L37
207: isWithdrawalFinalized[_l2BlockNumber][_l2MessageIndex] = true;
26: mapping(address => bool) public isAccessPublic;
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/common/AllowList.sol#L26
30: mapping(address => mapping(address => mapping(bytes4 => bool))) public hasSpecialAccessToCall;
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/common/AllowList.sol#L30
40: s.validators[_validator] = true;
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/zksync/DiamondInit.sol#L40
22: mapping(address => bool) securityCouncilMembers;
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/zksync/Storage.sol#L22
77: mapping(address => bool) validators;
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/zksync/Storage.sol#L77
68: diamondStorage.isFrozen = false;
98: diamondStorage.isFrozen = false;
83: diamondStorage.isFrozen = true;
133: isSystemContextLogProcessed = true;
72: getters.ignoreName = true;
78: getters.ignoreSymbol = true;
88: getters.ignoreDecimals = true;
Each slot saved can avoid an extra Gsset (20000 gas) for the first setting of the struct. Subsequent reads as well as writes have smaller gas savings
struct AppStorage { DiamondCutStorage diamondCutStorage; address governor; address pendingGovernor; mapping(address => bool) validators; Verifier verifier; uint256 totalBlocksExecuted; uint256 totalBlocksVerified; uint256 totalBlocksCommitted; mapping(uint256 => bytes32) storedBlockHashes; mapping(uint256 => bytes32) l2LogsRootHashes; PriorityQueue.Queue priorityQueue; IAllowList allowList; VerifierParams verifierParams; bytes32 l2BootloaderBytecodeHash; bytes32 l2DefaultAccountBytecodeHash; bool zkPorterIsAvailable; }
Saving 1 slot by changing to:
struct AppStorage { DiamondCutStorage diamondCutStorage; bool zkPorterIsAvailable; //@audit 8 bits address governor; //@audit 160 bits address pendingGovernor; mapping(address => bool) validators; Verifier verifier; uint256 totalBlocksExecuted; uint256 totalBlocksVerified; uint256 totalBlocksCommitted; mapping(uint256 => bytes32) storedBlockHashes; mapping(uint256 => bytes32) l2LogsRootHashes; PriorityQueue.Queue priorityQueue; IAllowList allowList; VerifierParams verifierParams; bytes32 l2BootloaderBytecodeHash; bytes32 l2DefaultAccountBytecodeHash; }
struct StoredBlockInfo { uint64 blockNumber; bytes32 blockHash; uint64 indexRepeatedStorageChanges; uint256 numberOfLayer1Txs; bytes32 priorityOperationsHash; bytes32 l2LogsTreeRoot; uint256 timestamp; bytes32 commitment; }
Saving 1 slot by changing to:
struct StoredBlockInfo { uint64 blockNumber; //@audit 64 bits uint64 indexRepeatedStorageChanges; //@audit 64 bits bytes32 blockHash; uint256 numberOfLayer1Txs; bytes32 priorityOperationsHash; bytes32 l2LogsTreeRoot; uint256 timestamp; bytes32 commitment; }
block.number and block.timestamp are added to the event information by default, so adding them manually will waste additional gas.
76: event BlockCommit(uint256 indexed blockNumber, bytes32 indexed blockHash, bytes32 indexed commitment);
82: event BlockExecution(uint256 indexed blockNumber, bytes32 indexed blockHash, bytes32 indexed commitment);
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.
12: uint256 constant L2_TO_L1_LOGS_COMMITMENT_BYTES = 4 + L2_TO_L1_LOG_SERIALIZE_SIZE * 512;
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/zksync/Config.sol#L12
23: uint256 constant INITIAL_STORAGE_CHANGES_COMMITMENT_BYTES = 4 + 64 * 4896;
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/zksync/Config.sol#L23
26: uint256 constant REPEATED_STORAGE_CHANGES_COMMITMENT_BYTES = 4 + 40 * 7787;
https://github.com/code-423n4/2022-10-zksync/tree/main/ethereum/contracts/zksync/Config.sol#L26
#0 - miladpiri
2022-11-23T14:55:14Z
It can be as high quality report!
#1 - GalloDaSballo
2022-11-25T01:35:57Z
GAS‑1 | ++i/i++ Should Be unchecked{++i}/unchecked{i++} When It Is Not Possible For Them To Overflow, As Is The Case When Used In For- And While-loops | 4 200
GAS‑2 | Splitting require() Statements That Use && Saves Gas | 2 6
GAS‑3 | abi.encode() is less efficient than abi.encodepacked() | 17
GAS‑4 | Use calldata instead of memory for function parameters | 3 Code doesn't compile with those changes
GAS‑5 | Public Functions To External | 2 Will not save gas as no calldata / memory is changed
GAS‑6 | Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead | 11 GAS‑7 | Optimize names to save gas | All in-scope contracts
GAS‑8 | Use uint256(1)/uint256(2) instead for true and false boolean states | 16 Will not change as much as in reentrancy guard
GAS‑9 | Structs can be packed into fewer storage slots | 2 4k
GAS‑10 | Superfluous event fields | 2 Roughly 100 gas
GAS‑11 | Do not calculate constants Dispute for months now and has been fixed years ago
#2 - GalloDaSballo
2022-11-25T01:36:06Z
4306
#3 - c4-judge
2022-12-03T19:02:43Z
GalloDaSballo marked the issue as grade-b