In L1ERC20Bridge.sol
function initialize(
bytes[] calldata _factoryDeps,
address _l2TokenFactory,
address _governor
) external reentrancyGuardInitializer {missing check require(_governor != address(0));
In Governance.sol,
/// @notice Change validator status (active or not active)
/// @param _validator Validator address
/// @param _active Active flag
function setValidator(address _validator, bool _active) external onlyGovernor {
if (s.validators[_validator] != _active) {
s.validators[_validator] = _active;
emit ValidatorStatusUpdate(_validator, _active);
}
}missing the check require(_validator != address(0));
In AllowList.sol, the admin can set newPendingOrder to admin himself.
the effect is that the contract becomes centralizated and there will be no pending admin capable of claiming the admin role given there is no expiration time for newPendingOwner setting.
function setPendingOwner(address _newPendingOwner) external onlyOwner {
// Save previous value into the stack to put it into the event later
address oldPendingOwner = pendingOwner;
if (oldPendingOwner != _newPendingOwner) {
// Change pending owner
pendingOwner = _newPendingOwner;
emit NewPendingOwner(oldPendingOwner, _newPendingOwner);
}
}We recommend add the check
require(owner != _newPendingOwner);In Governance.sol, the governor can set the newPendingGovernor to governor himself.
function setPendingGovernor(address _newPendingGovernor) external onlyGovernor {
// Save previous value into the stack to put it into the event later
address oldPendingGovernor = s.pendingGovernor;
if (oldPendingGovernor != _newPendingGovernor) {
// Change pending governor
s.pendingGovernor = _newPendingGovernor;
emit NewPendingGovernor(oldPendingGovernor, _newPendingGovernor);
}
}
We recommend add the check
require(s.governor != _newPendingGovernor);the contracts in scope use the floating progama
// SPDX-License-Identifier: MIT OR Apache-2.0
pragma solidity ^0.8.0;Floating pragmas should not be used. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.
in L1ETHBridge.sol
require(amount != 0);/// @notice Transfer ether from the contract to the receiver
/// @dev Reverts only if the transfer call failed
function _withdrawFunds(address _to, uint256 _amount) internal {
bool callSuccess;
// Low-level assembly call, to avoid any memory copying (save gas)
assembly {
callSuccess := call(gas(), _to, _amount, 0, 0, 0, 0)
}
require(callSuccess);
}
In Verifier.sol
require(vk.num_inputs == public_inputs.length);In Executor.sol
require(l2BlockTimestamp == _newBlock.timestamp);In L2StandERC20.sol
line 95
modifier onlyBridge() {
require(msg.sender == l2Bridge);
_;
}We recommend the project add error message to require statement. Otherwise if the transactoin in the require without error message, it would be very difficult for both user and developer to find out why the transaction revert.
In the current implementation, the admin can just set a validator address to true or to false immediately.
/// @notice Change validator status (active or not active)
/// @param _validator Validator address
/// @param _active Active flag
function setValidator(address _validator, bool _active) external onlyGovernor {
if (s.validators[_validator] != _active) {
s.validators[_validator] = _active;
emit ValidatorStatusUpdate(_validator, _active);
}
}In the function Allowlist.sol and Governance.sol contract, two step transfer is implemented for admin role. First, the admin set a pending admin, and the admin claim the admin role to prove that the new admin is intended and capable of managing the contract as a admin.
However, the same pattern is missing when setting the validator. Validator is really important because they are in charge of validatoring and committing the transaction state in zkSync system. Letting the validator claim the validator proves the validator is capable to process the transaction and has the intention to do.