From ed56f917de47f13ecf6baa3b97bd67929208635b Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Tue, 17 Sep 2024 11:08:04 -0400 Subject: [PATCH 1/8] create failing test for rollup creator --- src/bridge/ERC20Bridge.sol | 4 ++-- test/foundry/RollupCreator.t.sol | 37 +++++++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/bridge/ERC20Bridge.sol b/src/bridge/ERC20Bridge.sol index d2d3bffc..a30c894a 100644 --- a/src/bridge/ERC20Bridge.sol +++ b/src/bridge/ERC20Bridge.sol @@ -74,7 +74,7 @@ contract ERC20Bridge is AbsBridge, IERC20Bridge { function _transferFunds(uint256 amount) internal override { // fetch native token from Inbox - IERC20(nativeToken).safeTransferFrom(msg.sender, address(this), amount); + IERC20(nativeToken).safeTransferFrom(msg.sender, address(this), amount); // note: potential zero transfer } function _executeLowLevelCall( @@ -91,7 +91,7 @@ contract ERC20Bridge is AbsBridge, IERC20Bridge { } // first release native token - IERC20(_nativeToken).safeTransfer(to, value); + IERC20(_nativeToken).safeTransfer(to, value); // note: potential zero transfer success = true; // if there's data do additional contract call. Make sure that call is not used to diff --git a/test/foundry/RollupCreator.t.sol b/test/foundry/RollupCreator.t.sol index 68b8ebe2..df01dc24 100644 --- a/test/foundry/RollupCreator.t.sol +++ b/test/foundry/RollupCreator.t.sol @@ -20,6 +20,24 @@ import "@openzeppelin/contracts/access/Ownable.sol"; import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; import "@openzeppelin/contracts/token/ERC20/presets/ERC20PresetFixedSupply.sol"; +contract NoZeroTransferToken is ERC20PresetFixedSupply { + constructor( + string memory name_, + string memory symbol_, + uint256 initialSupply, + address owner + ) ERC20PresetFixedSupply(name_, symbol_, initialSupply, owner) {} + + function _transfer( + address from, + address to, + uint256 amount + ) internal virtual override { + require(amount > 0, "NoZeroTransferToken: zero transfer"); + super._transfer(from, to, amount); + } +} + contract RollupCreatorTest is Test { RollupCreator public rollupCreator; address public rollupOwner = makeAddr("rollupOwner"); @@ -241,11 +259,24 @@ contract RollupCreatorTest is Test { } function test_createErc20Rollup() public { - vm.startPrank(deployer); address nativeToken = address( new ERC20PresetFixedSupply("Appchain Token", "App", 1_000_000 ether, deployer) ); + _createERC20Rollup(nativeToken); + } + + function test_createErc20RollupNoZeroTransfer() public { + address nativeToken = address( + new NoZeroTransferToken("Appchain Token", "App", 1_000_000 ether, deployer) + ); + + _createERC20Rollup(nativeToken); + } + + function _createERC20Rollup(address nativeToken) internal { + vm.startPrank(deployer); + // deployment params ISequencerInbox.MaxTimeVariation memory timeVars = ISequencerInbox.MaxTimeVariation( ((60 * 60 * 24) / 15), @@ -297,6 +328,10 @@ contract RollupCreatorTest is Test { vm.stopPrank(); + _postCreateERC20RollupChecks(rollupAddress, batchPosterManager, nativeToken, validators, batchPosters); + } + + function _postCreateERC20RollupChecks(address rollupAddress, address batchPosterManager, address nativeToken, address[] memory validators, address[] memory batchPosters) internal { /// common checks /// rollup creator From b69072d0dbb8a52ffcffd06103768db6262a50ad Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Tue, 17 Sep 2024 11:13:01 -0400 Subject: [PATCH 2/8] annotate --- src/rollup/FactoryDeployerHelper.sol | 2 +- src/rollup/RollupCreator.sol | 2 +- src/rollup/RollupUserLogic.sol | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/rollup/FactoryDeployerHelper.sol b/src/rollup/FactoryDeployerHelper.sol index ba5732e0..acb852bd 100644 --- a/src/rollup/FactoryDeployerHelper.sol +++ b/src/rollup/FactoryDeployerHelper.sol @@ -19,7 +19,7 @@ contract FactoryDeployerHelper { address feeToken = IERC20Bridge(bridge).nativeToken(); uint256 amount = IDeployHelper(DEPLOY_HELPER).getDeploymentTotalCost(inbox, maxFeePerGas); - IERC20(feeToken).transferFrom(msg.sender, inbox, amount); + IERC20(feeToken).transferFrom(msg.sender, inbox, amount); // note: potential zero transfer IDeployHelper(DEPLOY_HELPER).perform(inbox, feeToken, maxFeePerGas); } } diff --git a/src/rollup/RollupCreator.sol b/src/rollup/RollupCreator.sol index fca3f245..871686ca 100644 --- a/src/rollup/RollupCreator.sol +++ b/src/rollup/RollupCreator.sol @@ -309,7 +309,7 @@ contract RollupCreator is Ownable { totalFeeNativeDenominated = totalFee * (10**(decimals - 18)); } - IERC20(_nativeToken).safeTransferFrom(msg.sender, _inbox, totalFeeNativeDenominated); + IERC20(_nativeToken).safeTransferFrom(msg.sender, _inbox, totalFeeNativeDenominated); // note: potential zero transfer // do it l2FactoriesDeployer.perform(_inbox, _nativeToken, _maxFeePerGas); diff --git a/src/rollup/RollupUserLogic.sol b/src/rollup/RollupUserLogic.sol index 95df077b..c99a1a7e 100644 --- a/src/rollup/RollupUserLogic.sol +++ b/src/rollup/RollupUserLogic.sol @@ -780,13 +780,13 @@ contract ERC20RollupUserLogic is AbsRollupUserLogic, IRollupUserERC20 { { uint256 amount = withdrawFunds(msg.sender); // This is safe because it occurs after all checks and effects - require(IERC20Upgradeable(stakeToken).transfer(msg.sender, amount), "TRANSFER_FAILED"); + require(IERC20Upgradeable(stakeToken).transfer(msg.sender, amount), "TRANSFER_FAILED"); // note: potential zero transfer return amount; } function receiveTokens(uint256 tokenAmount) private { require( - IERC20Upgradeable(stakeToken).transferFrom(msg.sender, address(this), tokenAmount), + IERC20Upgradeable(stakeToken).transferFrom(msg.sender, address(this), tokenAmount), // note: potential zero transfer "TRANSFER_FAIL" ); } From 3cd4f149bf2d0d33714beed7ed464cd462c918a9 Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Tue, 17 Sep 2024 13:06:44 -0400 Subject: [PATCH 3/8] fix rollup creator failure --- src/bridge/ERC20Bridge.sol | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/bridge/ERC20Bridge.sol b/src/bridge/ERC20Bridge.sol index a30c894a..0478cf25 100644 --- a/src/bridge/ERC20Bridge.sol +++ b/src/bridge/ERC20Bridge.sol @@ -74,7 +74,10 @@ contract ERC20Bridge is AbsBridge, IERC20Bridge { function _transferFunds(uint256 amount) internal override { // fetch native token from Inbox - IERC20(nativeToken).safeTransferFrom(msg.sender, address(this), amount); // note: potential zero transfer + if (amount == 0) { + return; + } + IERC20(nativeToken).safeTransferFrom(msg.sender, address(this), amount); } function _executeLowLevelCall( From cbf87e67b0380d0777846b727a299d64d54ed36e Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Tue, 17 Sep 2024 13:16:22 -0400 Subject: [PATCH 4/8] move test token --- test/foundry/RollupCreator.t.sol | 18 +----------------- test/foundry/util/NoZeroTransferToken.sol | 22 ++++++++++++++++++++++ 2 files changed, 23 insertions(+), 17 deletions(-) create mode 100644 test/foundry/util/NoZeroTransferToken.sol diff --git a/test/foundry/RollupCreator.t.sol b/test/foundry/RollupCreator.t.sol index df01dc24..50b09866 100644 --- a/test/foundry/RollupCreator.t.sol +++ b/test/foundry/RollupCreator.t.sol @@ -19,24 +19,8 @@ import "../../src/rollup/DeployHelper.sol"; import "@openzeppelin/contracts/access/Ownable.sol"; import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol"; import "@openzeppelin/contracts/token/ERC20/presets/ERC20PresetFixedSupply.sol"; +import {NoZeroTransferToken} from "./util/NoZeroTransferToken.sol"; -contract NoZeroTransferToken is ERC20PresetFixedSupply { - constructor( - string memory name_, - string memory symbol_, - uint256 initialSupply, - address owner - ) ERC20PresetFixedSupply(name_, symbol_, initialSupply, owner) {} - - function _transfer( - address from, - address to, - uint256 amount - ) internal virtual override { - require(amount > 0, "NoZeroTransferToken: zero transfer"); - super._transfer(from, to, amount); - } -} contract RollupCreatorTest is Test { RollupCreator public rollupCreator; diff --git a/test/foundry/util/NoZeroTransferToken.sol b/test/foundry/util/NoZeroTransferToken.sol new file mode 100644 index 00000000..89af9023 --- /dev/null +++ b/test/foundry/util/NoZeroTransferToken.sol @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity ^0.8.4; + +import "@openzeppelin/contracts/token/ERC20/presets/ERC20PresetFixedSupply.sol"; + +contract NoZeroTransferToken is ERC20PresetFixedSupply { + constructor( + string memory name_, + string memory symbol_, + uint256 initialSupply, + address owner + ) ERC20PresetFixedSupply(name_, symbol_, initialSupply, owner) {} + + function _transfer( + address from, + address to, + uint256 amount + ) internal virtual override { + require(amount > 0, "NoZeroTransferToken: zero transfer"); + super._transfer(from, to, amount); + } +} \ No newline at end of file From 511ca3e35d062e8584baa258079403903d75f3e6 Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Tue, 17 Sep 2024 15:14:22 -0400 Subject: [PATCH 5/8] test outbox execution with zero value --- src/bridge/ERC20Bridge.sol | 9 +- test/foundry/ERC20Outbox.t.sol | 186 ++++++++++++++++++--------------- 2 files changed, 106 insertions(+), 89 deletions(-) diff --git a/src/bridge/ERC20Bridge.sol b/src/bridge/ERC20Bridge.sol index 0478cf25..86a7a303 100644 --- a/src/bridge/ERC20Bridge.sol +++ b/src/bridge/ERC20Bridge.sol @@ -74,10 +74,9 @@ contract ERC20Bridge is AbsBridge, IERC20Bridge { function _transferFunds(uint256 amount) internal override { // fetch native token from Inbox - if (amount == 0) { - return; + if (amount > 0) { + IERC20(nativeToken).safeTransferFrom(msg.sender, address(this), amount); } - IERC20(nativeToken).safeTransferFrom(msg.sender, address(this), amount); } function _executeLowLevelCall( @@ -94,7 +93,9 @@ contract ERC20Bridge is AbsBridge, IERC20Bridge { } // first release native token - IERC20(_nativeToken).safeTransfer(to, value); // note: potential zero transfer + if (value > 0) { + IERC20(_nativeToken).safeTransfer(to, value); + } success = true; // if there's data do additional contract call. Make sure that call is not used to diff --git a/test/foundry/ERC20Outbox.t.sol b/test/foundry/ERC20Outbox.t.sol index 4c385a56..b603e95b 100644 --- a/test/foundry/ERC20Outbox.t.sol +++ b/test/foundry/ERC20Outbox.t.sol @@ -6,7 +6,7 @@ import "./ERC20Bridge.t.sol"; import "../../src/bridge/ERC20Bridge.sol"; import "../../src/bridge/ERC20Outbox.sol"; import "@openzeppelin/contracts/token/ERC20/ERC20.sol"; -import "@openzeppelin/contracts/token/ERC20/presets/ERC20PresetFixedSupply.sol"; +import {NoZeroTransferToken} from "./util/NoZeroTransferToken.sol"; contract ERC20OutboxTest is AbsOutboxTest { ERC20Outbox public erc20Outbox; @@ -17,7 +17,7 @@ contract ERC20OutboxTest is AbsOutboxTest { function setUp() public { // deploy token, bridge and outbox - nativeToken = new ERC20PresetFixedSupply("Appchain Token", "App", 1_000_000, address(this)); + nativeToken = new NoZeroTransferToken("Appchain Token", "App", 1_000_000, address(this)); bridge = IBridge(TestUtil.deployProxy(address(new ERC20Bridge()))); erc20Bridge = ERC20Bridge(address(bridge)); outbox = IOutbox(TestUtil.deployProxy(address(new ERC20Outbox()))); @@ -45,90 +45,11 @@ contract ERC20OutboxTest is AbsOutboxTest { } function test_executeTransaction() public { - // fund bridge with some tokens - vm.startPrank(user); - nativeToken.approve(address(bridge), 100); - nativeToken.transfer(address(bridge), 100); - vm.stopPrank(); - - // create msg receiver on L1 - ERC20L2ToL1Target target = new ERC20L2ToL1Target(); - target.setOutbox(address(outbox)); - - //// execute transaction - uint256 bridgeTokenBalanceBefore = nativeToken.balanceOf(address(bridge)); - uint256 targetTokenBalanceBefore = nativeToken.balanceOf(address(target)); - - bytes32[] memory proof = new bytes32[](1); - proof[0] = bytes32(0); - - uint256 withdrawalAmount = 15; - bytes memory data = abi.encodeWithSignature("receiveHook()"); - - uint256 index = 1; - bytes32 itemHash = outbox.calculateItemHash({ - l2Sender: user, - to: address(target), - l2Block: 300, - l1Block: 20, - l2Timestamp: 1234, - value: withdrawalAmount, - data: data - }); - bytes32 root = outbox.calculateMerkleRoot(proof, index, itemHash); - // store root - vm.prank(rollup); - outbox.updateSendRoot( - root, - bytes32(uint256(1)) - ); - - outbox.executeTransaction({ - proof: proof, - index: index, - l2Sender: user, - to: address(target), - l2Block: 300, - l1Block: 20, - l2Timestamp: 1234, - value: withdrawalAmount, - data: data - }); - - uint256 bridgeTokenBalanceAfter = nativeToken.balanceOf(address(bridge)); - assertEq( - bridgeTokenBalanceBefore - bridgeTokenBalanceAfter, - withdrawalAmount, - "Invalid bridge token balance" - ); - - uint256 targetTokenBalanceAfter = nativeToken.balanceOf(address(target)); - assertEq( - targetTokenBalanceAfter - targetTokenBalanceBefore, - withdrawalAmount, - "Invalid target token balance" - ); - - /// check context was properly set during execution - assertEq(uint256(target.l2Block()), 300, "Invalid l2Block"); - assertEq(uint256(target.timestamp()), 1234, "Invalid timestamp"); - assertEq(uint256(target.outputId()), index, "Invalid outputId"); - assertEq(target.sender(), user, "Invalid sender"); - assertEq(uint256(target.l1Block()), 20, "Invalid l1Block"); - assertEq(uint256(target.withdrawalAmount()), withdrawalAmount, "Invalid withdrawalAmount"); + _happyExecTx(15); + } - vm.expectRevert(abi.encodeWithSignature("AlreadySpent(uint256)", index)); - outbox.executeTransaction({ - proof: proof, - index: index, - l2Sender: user, - to: address(target), - l2Block: 300, - l1Block: 20, - l2Timestamp: 1234, - value: withdrawalAmount, - data: data - }); + function test_executeTransactionZeroValue() public { + _happyExecTx(0); } function test_executeTransaction_revert_CallTargetNotAllowed() public { @@ -467,6 +388,101 @@ contract ERC20OutboxTest is AbsOutboxTest { data: data }); } + + struct HappyExecTxStackVars { + uint256 bridgeTokenBalanceBefore; + uint256 targetTokenBalanceBefore; + ERC20L2ToL1Target target; + uint256 index; + bytes32 itemHash; + } + function _happyExecTx(uint256 withdrawalAmount) public { + HappyExecTxStackVars memory vars; + + // fund bridge with some tokens + vm.startPrank(user); + nativeToken.approve(address(bridge), 100); + nativeToken.transfer(address(bridge), 100); + vm.stopPrank(); + + // create msg receiver on L1 + vars.target = new ERC20L2ToL1Target(); + vars.target.setOutbox(address(outbox)); + + //// execute transaction + vars.bridgeTokenBalanceBefore = nativeToken.balanceOf(address(bridge)); + vars.targetTokenBalanceBefore = nativeToken.balanceOf(address(vars.target)); + + bytes32[] memory proof = new bytes32[](1); + proof[0] = bytes32(0); + + bytes memory data = abi.encodeWithSignature("receiveHook()"); + + vars.index = 1; + vars.itemHash = outbox.calculateItemHash({ + l2Sender: user, + to: address(vars.target), + l2Block: 300, + l1Block: 20, + l2Timestamp: 1234, + value: withdrawalAmount, + data: data + }); + bytes32 root = outbox.calculateMerkleRoot(proof, vars.index, vars.itemHash); + // store root + vm.prank(rollup); + outbox.updateSendRoot( + root, + bytes32(uint256(1)) + ); + + outbox.executeTransaction({ + proof: proof, + index: vars.index, + l2Sender: user, + to: address(vars.target), + l2Block: 300, + l1Block: 20, + l2Timestamp: 1234, + value: withdrawalAmount, + data: data + }); + + uint256 bridgeTokenBalanceAfter = nativeToken.balanceOf(address(bridge)); + assertEq( + vars.bridgeTokenBalanceBefore - bridgeTokenBalanceAfter, + withdrawalAmount, + "Invalid bridge token balance" + ); + + uint256 targetTokenBalanceAfter = nativeToken.balanceOf(address(vars.target)); + assertEq( + targetTokenBalanceAfter - vars.targetTokenBalanceBefore, + withdrawalAmount, + "Invalid target token balance" + ); + + /// check context was properly set during execution + assertEq(uint256(vars.target.l2Block()), 300, "Invalid l2Block"); + assertEq(uint256(vars.target.timestamp()), 1234, "Invalid timestamp"); + assertEq(uint256(vars.target.outputId()), vars.index, "Invalid outputId"); + assertEq(vars.target.sender(), user, "Invalid sender"); + assertEq(uint256(vars.target.l1Block()), 20, "Invalid l1Block"); + assertEq(uint256(vars.target.withdrawalAmount()), withdrawalAmount, "Invalid withdrawalAmount"); + + vm.expectRevert(abi.encodeWithSignature("AlreadySpent(uint256)", vars.index)); + outbox.executeTransaction({ + proof: proof, + index: vars.index, + l2Sender: user, + to: address(vars.target), + l2Block: 300, + l1Block: 20, + l2Timestamp: 1234, + value: withdrawalAmount, + data: data + }); + } } /** From 502323ed5d14d31a28fa57226c9d2ca8420442ed Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Tue, 17 Sep 2024 15:31:48 -0400 Subject: [PATCH 6/8] rule out a zero transfer --- src/rollup/RollupCreator.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rollup/RollupCreator.sol b/src/rollup/RollupCreator.sol index 871686ca..fca3f245 100644 --- a/src/rollup/RollupCreator.sol +++ b/src/rollup/RollupCreator.sol @@ -309,7 +309,7 @@ contract RollupCreator is Ownable { totalFeeNativeDenominated = totalFee * (10**(decimals - 18)); } - IERC20(_nativeToken).safeTransferFrom(msg.sender, _inbox, totalFeeNativeDenominated); // note: potential zero transfer + IERC20(_nativeToken).safeTransferFrom(msg.sender, _inbox, totalFeeNativeDenominated); // do it l2FactoriesDeployer.perform(_inbox, _nativeToken, _maxFeePerGas); From 265385fb7141cdecb8ddd115b841caa57e642fda Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Tue, 17 Sep 2024 15:35:23 -0400 Subject: [PATCH 7/8] rule out rollup zero transfers --- src/rollup/RollupUserLogic.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/rollup/RollupUserLogic.sol b/src/rollup/RollupUserLogic.sol index c99a1a7e..95df077b 100644 --- a/src/rollup/RollupUserLogic.sol +++ b/src/rollup/RollupUserLogic.sol @@ -780,13 +780,13 @@ contract ERC20RollupUserLogic is AbsRollupUserLogic, IRollupUserERC20 { { uint256 amount = withdrawFunds(msg.sender); // This is safe because it occurs after all checks and effects - require(IERC20Upgradeable(stakeToken).transfer(msg.sender, amount), "TRANSFER_FAILED"); // note: potential zero transfer + require(IERC20Upgradeable(stakeToken).transfer(msg.sender, amount), "TRANSFER_FAILED"); return amount; } function receiveTokens(uint256 tokenAmount) private { require( - IERC20Upgradeable(stakeToken).transferFrom(msg.sender, address(this), tokenAmount), // note: potential zero transfer + IERC20Upgradeable(stakeToken).transferFrom(msg.sender, address(this), tokenAmount), "TRANSFER_FAIL" ); } From b11f03bd2c004a42d90dfed11b30ef9c8cbb7047 Mon Sep 17 00:00:00 2001 From: Henry <11198460+godzillaba@users.noreply.github.com> Date: Wed, 18 Sep 2024 15:40:25 -0400 Subject: [PATCH 8/8] rule out FactoryDeployerHelper --- src/rollup/FactoryDeployerHelper.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rollup/FactoryDeployerHelper.sol b/src/rollup/FactoryDeployerHelper.sol index acb852bd..ba5732e0 100644 --- a/src/rollup/FactoryDeployerHelper.sol +++ b/src/rollup/FactoryDeployerHelper.sol @@ -19,7 +19,7 @@ contract FactoryDeployerHelper { address feeToken = IERC20Bridge(bridge).nativeToken(); uint256 amount = IDeployHelper(DEPLOY_HELPER).getDeploymentTotalCost(inbox, maxFeePerGas); - IERC20(feeToken).transferFrom(msg.sender, inbox, amount); // note: potential zero transfer + IERC20(feeToken).transferFrom(msg.sender, inbox, amount); IDeployHelper(DEPLOY_HELPER).perform(inbox, feeToken, maxFeePerGas); } }