Findings for the wstETH bridging Op review
- ENV-setup deploy (it's better to have verifiable transparent source of all addresses and constants)
- To fit the transparency requirements before the deployment might be added ".env.optimis.wsteth" and ".env.arbitrum.wsteth" files with fixed variables for wstETH deployment. These files will be used as seed files for the actual .env file
- https://github.com/lidofinance/lido-l2/tree/develop/contracts/optimism#attack-on-l1crossdomainmessenger
Maybe resolve the issue by whitelisting implementations and/or monitoring with Forta (request to the Automation team)
- Need to remove
IERC20Metadata
(reproducible for the develop
branch).
- Maybe it is worth to add the additional events for
_burn
and _mint
(optimism ERC20Core
)?
https://github.com/lidofinance/lido-l2/blob/develop/contracts/token/ERC20Core.sol#L138
https://github.com/lidofinance/lido-l2/blob/develop/contracts/token/ERC20Core.sol#L150
- I think such events aren't needed in the
ERC20Core
methods cause it's not the common practice. But we can add BridgeMinted(address indexed account, uint amount)
and BridgeBurned(address indexed account, uint amount)
events in the ERC20Bridged contract. Or not add any additional events because we can clearly find when mint and burn happened using the Transfer(address indexed from, address indexed to, uint256 value)
events. It also will decrease the gas costs, but on L2, it's not so critical.
- Multichain network name
https://github.com/lidofinance/lido-l2/blob/develop/scripts/optimism/finalize-message.ts#L9
Should be "optimism"?
- Unclear intention
https://github.com/lidofinance/lido-l2/blob/develop/scripts/arbitrum/deploy-gateway.ts#L53
It's slightly vague, why we are moving from step 1 to step 3 (skipping step 2)
- Why do we have chain id #42 for optimism?
https://github.com/lidofinance/lido-l2/blob/develop/scripts/optimism/finalize-message.ts#L14
- A few typos:
- in
aren
in line @dev Validates that withdrawals aren enabled
recieve
in CrossDomainEnabled.sol
- in README: fixed in git patch
-
_setupRole() is deprecated according to OpenZeppelin docs
-
Typos: