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: 6/24
Findings: 1
Award: $2,102.32
🌟 Selected for report: 0
🚀 Solo Findings: 0
2102.3184 USDC - $2,102.32
If a variable gets configured with address zero, failure to immediately reset the value can result in unexpected behavior for the project.
initialize() is susceptible to frontrunFor 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.
https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1EthBridge.sol#L56
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.
https://github.com/code-423n4/2022-10-zksync/blob/main/zksync/contracts/bridge/L2ERC20Bridge.sol#L23
L1ERC20Bridge if deposit failsThe 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.
deposit()claimFailedDeposit(), the amount transfered back won't account for an increased balanceOFTake 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.
https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1EthBridge.sol#L60
https://github.com/code-423n4/2022-10-zksync/blob/main/ethereum/contracts/bridge/L1EthBridge.sol#L65
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.
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.
https://github.com/code-423n4/2022-10-zksync/blob/main/zksync/contracts/bridge/L2ETHBridge.sol#L79
#0 - GalloDaSballo
2022-11-22T02:10:49Z
L
L but it's actually safe (but awarded for fairness)
Invalid
L
R as the function is a create2 of known code, but perhaps worth changing
NC
##Â [07] Outdated OpenZeppelin version R
Disputing
Disputing, please provide the instance to the issue caused by the optimizer and we'll award higher severity
NC
NC
R
NC
NC
NC
##Â [16] Inconsistent use of named return variables R
R
#1 - GalloDaSballo
2022-12-03T00:55:48Z
3L 5R 6NC
#2 - c4-judge
2022-12-03T19:08:42Z
GalloDaSballo marked the issue as grade-a