From 2e507b2c6e794f9e8813502265fa334065efb42f Mon Sep 17 00:00:00 2001 From: AgusDuha <81362284+agusduha@users.noreply.github.com> Date: Wed, 2 Oct 2024 16:12:59 -0300 Subject: [PATCH] fix: refactor zero check (#76) --- packages/contracts-bedrock/semver-lock.json | 8 ++--- .../abi/OptimismSuperchainERC20.json | 4 +-- .../snapshots/abi/SuperchainERC20Bridge.json | 5 ++++ .../src/L2/SuperchainERC20.sol | 8 ++--- .../src/L2/SuperchainERC20Bridge.sol | 2 ++ .../interfaces/IOptimismSuperchainERC20.sol | 3 ++ .../src/L2/interfaces/ISuperchainERC20.sol | 7 ++--- .../L2/interfaces/ISuperchainERC20Bridge.sol | 3 ++ .../test/L2/OptimismSuperchainERC20.t.sol | 5 ++-- .../test/L2/SuperchainERC20.t.sol | 30 ++++--------------- .../test/L2/SuperchainERC20Bridge.t.sol | 10 +++++++ 11 files changed, 40 insertions(+), 45 deletions(-) diff --git a/packages/contracts-bedrock/semver-lock.json b/packages/contracts-bedrock/semver-lock.json index 57e415387b68..e794d31001a1 100644 --- a/packages/contracts-bedrock/semver-lock.json +++ b/packages/contracts-bedrock/semver-lock.json @@ -116,7 +116,7 @@ "sourceCodeHash": "0x4b806cc85cead74c8df34ab08f4b6c6a95a1a387a335ec8a7cb2de4ea4e1cf41" }, "src/L2/OptimismSuperchainERC20.sol": { - "initCodeHash": "0xfdb4acd9496a7d3949f71e7e98786ff909730a8ad62d33cf7e29765dceecc6db", + "initCodeHash": "0xc6452d9aef6d76bdc789f3cddac6862658a481c619e6a2e7a74f6d61147f927b", "sourceCodeHash": "0x2502433e4b622e1697ca071f91a95b08fa40fdb03bfd958c44b2033a47df2010" }, "src/L2/OptimismSuperchainERC20Beacon.sol": { @@ -133,11 +133,11 @@ }, "src/L2/SuperchainERC20.sol": { "initCodeHash": "0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470", - "sourceCodeHash": "0xddd32f6332510e63d8c98d70321e058b71eda02e6b32a9b6e41540c58d1e653e" + "sourceCodeHash": "0x9bc2e208774eb923894dbe391a5038a6189d7d36c202f4bf3e2c4dd332b0adf0" }, "src/L2/SuperchainERC20Bridge.sol": { - "initCodeHash": "0x77d3173e1f269f6bf57f85685abecb4979a7d7d3c672c7afa2a648b66228122f", - "sourceCodeHash": "0xf0749a0b9366a06981d2a8f66a55ce1a37e3d5d7dd77704f618741c18cd79009" + "initCodeHash": "0xa21232df1d7239fd20e7eaa320cfc91efc76343c93d833d8060a58b54ac5c8bf", + "sourceCodeHash": "0x83188d878ce0b2890a7f7f41d09a8807f94a126e0ea274f0dac8b93f77217d3b" }, "src/L2/SuperchainWETH.sol": { "initCodeHash": "0xf30071df59d85e0e8a552845031aca8d6f0261762e1b4ea1b28ff30379eaa20e", diff --git a/packages/contracts-bedrock/snapshots/abi/OptimismSuperchainERC20.json b/packages/contracts-bedrock/snapshots/abi/OptimismSuperchainERC20.json index 1c7e5b2f4e8e..7c24b3fe0065 100644 --- a/packages/contracts-bedrock/snapshots/abi/OptimismSuperchainERC20.json +++ b/packages/contracts-bedrock/snapshots/abi/OptimismSuperchainERC20.json @@ -488,7 +488,7 @@ "type": "uint256" } ], - "name": "SuperchainBurn", + "name": "SuperchainBurnt", "type": "event" }, { @@ -507,7 +507,7 @@ "type": "uint256" } ], - "name": "SuperchainMint", + "name": "SuperchainMinted", "type": "event" }, { diff --git a/packages/contracts-bedrock/snapshots/abi/SuperchainERC20Bridge.json b/packages/contracts-bedrock/snapshots/abi/SuperchainERC20Bridge.json index ecea3d7d420c..ed7ff2ba5129 100644 --- a/packages/contracts-bedrock/snapshots/abi/SuperchainERC20Bridge.json +++ b/packages/contracts-bedrock/snapshots/abi/SuperchainERC20Bridge.json @@ -151,5 +151,10 @@ "inputs": [], "name": "InvalidCrossDomainSender", "type": "error" + }, + { + "inputs": [], + "name": "ZeroAddress", + "type": "error" } ] \ No newline at end of file diff --git a/packages/contracts-bedrock/src/L2/SuperchainERC20.sol b/packages/contracts-bedrock/src/L2/SuperchainERC20.sol index c67cb8240621..6c48b231baaf 100644 --- a/packages/contracts-bedrock/src/L2/SuperchainERC20.sol +++ b/packages/contracts-bedrock/src/L2/SuperchainERC20.sol @@ -27,21 +27,17 @@ abstract contract SuperchainERC20 is ERC20, ISuperchainERC20Extension, ISemver { /// @param _to Address to mint tokens to. /// @param _amount Amount of tokens to mint. function __superchainMint(address _to, uint256 _amount) external virtual onlySuperchainERC20Bridge { - if (_to == address(0)) revert ZeroAddress(); - _mint(_to, _amount); - emit SuperchainMint(_to, _amount); + emit SuperchainMinted(_to, _amount); } /// @notice Allows the SuperchainERC20Bridge to burn tokens. /// @param _from Address to burn tokens from. /// @param _amount Amount of tokens to burn. function __superchainBurn(address _from, uint256 _amount) external virtual onlySuperchainERC20Bridge { - if (_from == address(0)) revert ZeroAddress(); - _burn(_from, _amount); - emit SuperchainBurn(_from, _amount); + emit SuperchainBurnt(_from, _amount); } } diff --git a/packages/contracts-bedrock/src/L2/SuperchainERC20Bridge.sol b/packages/contracts-bedrock/src/L2/SuperchainERC20Bridge.sol index 103fe5794a1d..9d13de80f4ca 100644 --- a/packages/contracts-bedrock/src/L2/SuperchainERC20Bridge.sol +++ b/packages/contracts-bedrock/src/L2/SuperchainERC20Bridge.sol @@ -29,6 +29,8 @@ contract SuperchainERC20Bridge is ISuperchainERC20Bridge { /// @param _amount Amount of tokens to send. /// @param _chainId Chain ID of the destination chain. function sendERC20(address _token, address _to, uint256 _amount, uint256 _chainId) external { + if (_to == address(0)) revert ZeroAddress(); + ISuperchainERC20(_token).__superchainBurn(msg.sender, _amount); bytes memory message = abi.encodeCall(this.relayERC20, (_token, msg.sender, _to, _amount)); diff --git a/packages/contracts-bedrock/src/L2/interfaces/IOptimismSuperchainERC20.sol b/packages/contracts-bedrock/src/L2/interfaces/IOptimismSuperchainERC20.sol index 3000f0d85be0..a887ecf0e030 100644 --- a/packages/contracts-bedrock/src/L2/interfaces/IOptimismSuperchainERC20.sol +++ b/packages/contracts-bedrock/src/L2/interfaces/IOptimismSuperchainERC20.sol @@ -8,6 +8,9 @@ import { IERC20Solady } from "src/vendor/interfaces/IERC20Solady.sol"; /// @title IOptimismSuperchainERC20Errors /// @notice Interface containing the errors added in the OptimismSuperchainERC20 implementation. interface IOptimismSuperchainERC20Errors { + /// @notice Thrown when attempting to perform an operation and the account is the zero address. + error ZeroAddress(); + /// @notice Thrown when attempting to mint or burn tokens and the function caller is not the L2StandardBridge error OnlyL2StandardBridge(); } diff --git a/packages/contracts-bedrock/src/L2/interfaces/ISuperchainERC20.sol b/packages/contracts-bedrock/src/L2/interfaces/ISuperchainERC20.sol index 9ae84f6e7d2f..f63e8a6abb6d 100644 --- a/packages/contracts-bedrock/src/L2/interfaces/ISuperchainERC20.sol +++ b/packages/contracts-bedrock/src/L2/interfaces/ISuperchainERC20.sol @@ -7,9 +7,6 @@ import { IERC20Solady } from "src/vendor/interfaces/IERC20Solady.sol"; /// @title ISuperchainERC20Errors /// @notice Interface containing the errors added in the SuperchainERC20 implementation. interface ISuperchainERC20Errors { - /// @notice Thrown when attempting to perform an operation and the account is the zero address. - error ZeroAddress(); - /// @notice Thrown when attempting to mint or burn tokens and the function caller is not the SuperchainERC20Bridge. error OnlySuperchainERC20Bridge(); } @@ -20,12 +17,12 @@ interface ISuperchainERC20Extension is ISuperchainERC20Errors { /// @notice Emitted whenever tokens are minted for by the SuperchainERC20Bridge. /// @param account Address of the account tokens are being minted for. /// @param amount Amount of tokens minted. - event SuperchainMint(address indexed account, uint256 amount); + event SuperchainMinted(address indexed account, uint256 amount); /// @notice Emitted whenever tokens are burned by the SuperchainERC20Bridge. /// @param account Address of the account tokens are being burned from. /// @param amount Amount of tokens burned. - event SuperchainBurn(address indexed account, uint256 amount); + event SuperchainBurnt(address indexed account, uint256 amount); /// @notice Allows the SuperchainERC20Bridge to mint tokens. /// @param _to Address to mint tokens to. diff --git a/packages/contracts-bedrock/src/L2/interfaces/ISuperchainERC20Bridge.sol b/packages/contracts-bedrock/src/L2/interfaces/ISuperchainERC20Bridge.sol index 6798f01f6f3a..8a3da87633bb 100644 --- a/packages/contracts-bedrock/src/L2/interfaces/ISuperchainERC20Bridge.sol +++ b/packages/contracts-bedrock/src/L2/interfaces/ISuperchainERC20Bridge.sol @@ -6,6 +6,9 @@ import { ISemver } from "src/universal/interfaces/ISemver.sol"; /// @title ISuperchainERC20Bridge /// @notice Interface for the SuperchainERC20Bridge contract. interface ISuperchainERC20Bridge is ISemver { + /// @notice Thrown when attempting to perform an operation and the account is the zero address. + error ZeroAddress(); + /// @notice Thrown when attempting to relay a message and the function caller (msg.sender) is not /// L2ToL2CrossDomainMessenger. error CallerNotL2ToL2CrossDomainMessenger(); diff --git a/packages/contracts-bedrock/test/L2/OptimismSuperchainERC20.t.sol b/packages/contracts-bedrock/test/L2/OptimismSuperchainERC20.t.sol index 115e6f268bb2..239c785b51c0 100644 --- a/packages/contracts-bedrock/test/L2/OptimismSuperchainERC20.t.sol +++ b/packages/contracts-bedrock/test/L2/OptimismSuperchainERC20.t.sol @@ -20,7 +20,6 @@ import { IOptimismSuperchainERC20Extension, IOptimismSuperchainERC20Errors } from "src/L2/interfaces/IOptimismSuperchainERC20.sol"; -import { ISuperchainERC20Errors } from "src/L2/interfaces/ISuperchainERC20.sol"; /// @title OptimismSuperchainERC20Test /// @notice Contract for testing the OptimismSuperchainERC20 contract. @@ -133,7 +132,7 @@ contract OptimismSuperchainERC20Test is Test { /// @notice Tests the `mint` function reverts when the amount is zero. function testFuzz_mint_zeroAddressTo_reverts(uint256 _amount) public { // Expect the revert with `ZeroAddress` selector - vm.expectRevert(ISuperchainERC20Errors.ZeroAddress.selector); + vm.expectRevert(IOptimismSuperchainERC20Errors.ZeroAddress.selector); // Call the `mint` function with the zero address vm.prank(L2_BRIDGE); @@ -182,7 +181,7 @@ contract OptimismSuperchainERC20Test is Test { /// @notice Tests the `burn` function reverts when the amount is zero. function testFuzz_burn_zeroAddressFrom_reverts(uint256 _amount) public { // Expect the revert with `ZeroAddress` selector - vm.expectRevert(ISuperchainERC20Errors.ZeroAddress.selector); + vm.expectRevert(IOptimismSuperchainERC20Errors.ZeroAddress.selector); // Call the `burn` function with the zero address vm.prank(L2_BRIDGE); diff --git a/packages/contracts-bedrock/test/L2/SuperchainERC20.t.sol b/packages/contracts-bedrock/test/L2/SuperchainERC20.t.sol index 372f85c061fa..bc5785901d14 100644 --- a/packages/contracts-bedrock/test/L2/SuperchainERC20.t.sol +++ b/packages/contracts-bedrock/test/L2/SuperchainERC20.t.sol @@ -62,16 +62,6 @@ contract SuperchainERC20Test is Test { superchainERC20.__superchainMint(_to, _amount); } - /// @notice Tests the `mint` function reverts when the amount is zero. - function testFuzz___superchainMint_zeroAddressTo_reverts(uint256 _amount) public { - // Expect the revert with `ZeroAddress` selector - vm.expectRevert(ISuperchainERC20Errors.ZeroAddress.selector); - - // Call the `mint` function with the zero address - vm.prank(SUPERCHAIN_ERC20_BRIDGE); - superchainERC20.__superchainMint({ _to: ZERO_ADDRESS, _amount: _amount }); - } - /// @notice Tests the `mint` succeeds and emits the `Mint` event. function testFuzz___superchainMint_succeeds(address _to, uint256 _amount) public { // Ensure `_to` is not the zero address @@ -85,9 +75,9 @@ contract SuperchainERC20Test is Test { vm.expectEmit(address(superchainERC20)); emit IERC20.Transfer(ZERO_ADDRESS, _to, _amount); - // Look for the emit of the `SuperchainMint` event + // Look for the emit of the `SuperchainMinted` event vm.expectEmit(address(superchainERC20)); - emit ISuperchainERC20Extension.SuperchainMint(_to, _amount); + emit ISuperchainERC20Extension.SuperchainMinted(_to, _amount); // Call the `mint` function with the bridge caller vm.prank(SUPERCHAIN_ERC20_BRIDGE); @@ -117,17 +107,7 @@ contract SuperchainERC20Test is Test { superchainERC20.__superchainBurn(_from, _amount); } - /// @notice Tests the `burn` function reverts when the amount is zero. - function testFuzz___superchainBurn_zeroAddressFrom_reverts(uint256 _amount) public { - // Expect the revert with `ZeroAddress` selector - vm.expectRevert(ISuperchainERC20Errors.ZeroAddress.selector); - - // Call the `burn` function with the zero address - vm.prank(SUPERCHAIN_ERC20_BRIDGE); - superchainERC20.__superchainBurn({ _from: ZERO_ADDRESS, _amount: _amount }); - } - - /// @notice Tests the `burn` burns the amount and emits the `Burn` event. + /// @notice Tests the `burn` burns the amount and emits the `SuperchainBurnt` event. function testFuzz___superchainBurn_succeeds(address _from, uint256 _amount) public { // Ensure `_from` is not the zero address vm.assume(_from != ZERO_ADDRESS); @@ -144,9 +124,9 @@ contract SuperchainERC20Test is Test { vm.expectEmit(address(superchainERC20)); emit IERC20.Transfer(_from, ZERO_ADDRESS, _amount); - // Look for the emit of the `Burn` event + // Look for the emit of the `SuperchainBurnt` event vm.expectEmit(address(superchainERC20)); - emit ISuperchainERC20Extension.SuperchainBurn(_from, _amount); + emit ISuperchainERC20Extension.SuperchainBurnt(_from, _amount); // Call the `burn` function with the bridge caller vm.prank(SUPERCHAIN_ERC20_BRIDGE); diff --git a/packages/contracts-bedrock/test/L2/SuperchainERC20Bridge.t.sol b/packages/contracts-bedrock/test/L2/SuperchainERC20Bridge.t.sol index 3d1a82fffa76..7ec72e508dad 100644 --- a/packages/contracts-bedrock/test/L2/SuperchainERC20Bridge.t.sol +++ b/packages/contracts-bedrock/test/L2/SuperchainERC20Bridge.t.sol @@ -49,6 +49,16 @@ contract SuperchainERC20BridgeTest is Bridge_Initializer { vm.expectCall(_receiver, _calldata); } + /// @notice Tests the `sendERC20` function reverts when the address `_to` is zero. + function testFuzz_sendERC20_zeroAddressTo_reverts(address _sender, uint256 _amount, uint256 _chainId) public { + // Expect the revert with `ZeroAddress` selector + vm.expectRevert(ISuperchainERC20Bridge.ZeroAddress.selector); + + // Call the `sendERC20` function with the zero address as `_to` + vm.prank(_sender); + superchainERC20Bridge.sendERC20(address(superchainERC20), ZERO_ADDRESS, _amount, _chainId); + } + /// @notice Tests the `sendERC20` function burns the sender tokens, sends the message, and emits the `SendERC20` /// event. function testFuzz_sendERC20_succeeds(address _sender, address _to, uint256 _amount, uint256 _chainId) external {