If a variable gets configured with address zero, failure to immediately reset the value can result in unexpected behavior for the project.
For the L1 Eth bridge, the comment on L47 indictates to call initalize() before a potential attacker (similar pattern in L1 ERC20 bridge). However, there's no access restriction on initializa() - anyone can call (and only once) after deployment.
Conside adding access control on initialize(). If possible, add a check so that only the address that deploys the contract (and executes constructor()) can call initialize().
Consider adding a __gap[] storage variable to allow for new storage variables in later versions. Example.
See this link for a description of this storage variable issue. Adding the variable now protects against forgetting to add it in the future.
The balanceOf rebase tokens (e.g. Aave aTokens) are increased over time. If such tokens are used in deposit() and the transaction fails on the L2 side, calls to claimFailedDeposit() won't account for the increased values. The value would only be meaningful if the deposit is large and the claim is made a reasonable time after the deposit.
- User deposits a rebase ERC20 token, by calling
deposit() - The transaction fails on L2.
- User claims the failed deposit, by calling
claimFailedDeposit(), theamounttransfered back won't account for an increasedbalanceOF
Take the increased balanceOf into account when computing the amount to be returned in claimFailedDeposit(), to support rebase tokens.
Alternatively, if the gas costs and added code commplexity to add this functionality are considered unnecessary by the project, consider documenting that values increased by rebase tokens will be lost for ERC20 L1-L2 claims for failed deposits.
On L1ERC20Bridge.initialize() and L1EthBridge.initialize(), the state variable l2Bridge is updated after the abi.encodeCall() external call. Consider updating the state before external calls to follow the checks-effects-interactions pattern.
Locking the pragma helps to ensure that contracts do not accidentally get deployed using an outdated compiler version.
Note that pragma statements can be allowed to float when a contract is intended for consumption by other developers, as in the case with contracts in a library or a package.
The project is using OpenZeppelin v4.6.0. Consider updating to the latest version: v4.7.3, to ensure the library contracts contain the latest security fixes.
As described on the solidity documentation. "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.
Even if the code is expected to be unreacheable, consider using a require statement or a custom error instead of assert.
[09] Avoid using the optimizer if possible, due to it's potential security bugs which can affect the contracts in scope
Optimizations are being actively developed and are not considered safe. Check the following audit recommending agaist deployments with the optimizer enabled (item 3 in the audit document).
If possible, consider measuring and documenting the tradeoffs when enabling the optimizer.
Some external functions are missing unit tests (e.g. DiamondCut.approveEmergencyDiamondCutAsSecurityCouncilMember() and functions inside bridge contracts). Consider testing all code paths to improve the coverage.
Todos should be resolved before deployment
https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/zksync/Config.sol#L28
Consider adding natspec/comments on all functions to improve code documentation and readability.
The solidity documentation recommends the following order for functions:
constructor receive function (if exists) fallback function (if exists) external public internal private
The instances bellow highlight a block with internal above public.
Executor._calculateBlockHash() is not used.
Some functions return named variables, others return explicit values.
Following function return explicit value
Following function return return a named variable
Consider adopting a consistent approach to return values throughout the codebase by removing all named return variables, explicitly declaring them as local variables, and adding the necessary return statements where appropriate. This would improve both the explicitness and readability of the code, and it may also help reduce regressions during future code refactors.