From b8b711e347a93b960e0c2c9d54e8b7d2bf1eb849 Mon Sep 17 00:00:00 2001 From: Chris Buckland Date: Wed, 2 Aug 2023 14:06:58 +0200 Subject: [PATCH 01/16] Removed whenNotPaused from deposit withdrawal --- src/rollup/RollupUserLogic.sol | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/rollup/RollupUserLogic.sol b/src/rollup/RollupUserLogic.sol index bd16ad5e..0617f363 100644 --- a/src/rollup/RollupUserLogic.sol +++ b/src/rollup/RollupUserLogic.sol @@ -234,7 +234,7 @@ abstract contract AbsRollupUserLogic is * and move it to the desired node. * @param stakerAddress Address of the staker whose stake is refunded */ - function returnOldDeposit(address stakerAddress) external override onlyValidator whenNotPaused { + function returnOldDeposit(address stakerAddress) external override onlyValidator { require(latestStakedNode(stakerAddress) <= latestConfirmed(), "TOO_RECENT"); requireUnchallengedStaker(stakerAddress); withdrawStaker(stakerAddress); @@ -258,7 +258,7 @@ abstract contract AbsRollupUserLogic is * @notice Reduce the amount staked for the sender (difference between initial amount staked and target is creditted back to the sender). * @param target Target amount of stake for the staker. If this is below the current minimum, it will be set to minimum instead */ - function reduceDeposit(uint256 target) external onlyValidator whenNotPaused { + function reduceDeposit(uint256 target) external onlyValidator { requireUnchallengedStaker(msg.sender); uint256 currentRequired = currentRequiredStake(); if (target < currentRequired) { @@ -659,7 +659,7 @@ contract RollupUserLogic is AbsRollupUserLogic, IRollupUser { /** * @notice Withdraw uncommitted funds owned by sender from the rollup chain */ - function withdrawStakerFunds() external override onlyValidator whenNotPaused returns (uint256) { + function withdrawStakerFunds() external override onlyValidator returns (uint256) { uint256 amount = withdrawFunds(msg.sender); // This is safe because it occurs after all checks and effects // solhint-disable-next-line avoid-low-level-calls @@ -731,7 +731,7 @@ contract ERC20RollupUserLogic is AbsRollupUserLogic, IRollupUserERC20 { /** * @notice Withdraw uncommitted funds owned by sender from the rollup chain */ - function withdrawStakerFunds() external override onlyValidator whenNotPaused returns (uint256) { + function withdrawStakerFunds() external override onlyValidator returns (uint256) { 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"); From 0139283b2db6c1f85eb1cfff8277875c3eb1dda6 Mon Sep 17 00:00:00 2001 From: gzeon Date: Mon, 7 Aug 2023 12:38:20 +0100 Subject: [PATCH 02/16] test: withdraw on pause --- test/contract/arbRollup.spec.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/contract/arbRollup.spec.ts b/test/contract/arbRollup.spec.ts index fca5eb97..5740b92a 100644 --- a/test/contract/arbRollup.spec.ts +++ b/test/contract/arbRollup.spec.ts @@ -823,6 +823,7 @@ describe('ArbRollup', () => { .connect(validators[1]) .addToDeposit(await validators[1].getAddress(), { value: 5 }) + await rollupAdmin.pause() await rollup.connect(validators[1]).reduceDeposit(5) const prevBalance = await validators[1].getBalance() @@ -849,6 +850,7 @@ describe('ArbRollup', () => { .connect(validators[1]) .returnOldDeposit(await validators[1].getAddress()) // all stake is now removed + await rollupAdmin.resume() }) it('should allow removing zombies', async function () { From 36c568b38ecdd631db79148150d9eb06fc0c1149 Mon Sep 17 00:00:00 2001 From: Chris Buckland Date: Fri, 22 Sep 2023 16:10:08 +0200 Subject: [PATCH 03/16] Added test for force refunding staker --- test/contract/arbRollup.spec.ts | 112 ++++++++++++++++++-------------- 1 file changed, 64 insertions(+), 48 deletions(-) diff --git a/test/contract/arbRollup.spec.ts b/test/contract/arbRollup.spec.ts index 5740b92a..24132b29 100644 --- a/test/contract/arbRollup.spec.ts +++ b/test/contract/arbRollup.spec.ts @@ -789,69 +789,85 @@ describe('ArbRollup', () => { await rollup.confirmNextNode(challengerNode) }) - it('should add and remove stakes correctly', async function () { - /* - RollupUser functions that alter stake and their respective Core logic + it('allow force refund staker with pending node', async function () { + await (await rollupAdmin.pause()).wait(); + await (await rollupAdmin.forceRefundStaker([await validators[1].getAddress()])).wait() + await (await rollup.rollup.connect(validators[1]).withdrawStakerFunds()).wait() + await (await rollupAdmin.resume()).wait(); - user: newStake - core: createNewStake + const postWithdrawablefunds = await rollup.rollup.withdrawableFunds( + await validators[1].getAddress() + ) + expect(postWithdrawablefunds, "withdrawable funds").to.equal(0) + const stake = await rollup.rollup.amountStaked( + await validators[1].getAddress() + ) + expect(stake, "amount staked").to.equal(0) + }) - user: addToDeposit - core: increaseStakeBy + // it('should add and remove stakes correctly', async function () { + // /* + // RollupUser functions that alter stake and their respective Core logic - user: reduceDeposit - core: reduceStakeTo + // user: newStake + // core: createNewStake - user: returnOldDeposit - core: withdrawStaker + // user: addToDeposit + // core: increaseStakeBy - user: withdrawStakerFunds - core: withdrawFunds - */ + // user: reduceDeposit + // core: reduceStakeTo - const initialStake = await rollup.rollup.amountStaked( - await validators[1].getAddress() - ) + // user: returnOldDeposit + // core: withdrawStaker - await rollup.connect(validators[1]).reduceDeposit(initialStake) + // user: withdrawStakerFunds + // core: withdrawFunds + // */ - await expect( - rollup.connect(validators[1]).reduceDeposit(initialStake.add(1)) - ).to.be.revertedWith('TOO_LITTLE_STAKE') + // const initialStake = await rollup.rollup.amountStaked( + // await validators[1].getAddress() + // ) - await rollup - .connect(validators[1]) - .addToDeposit(await validators[1].getAddress(), { value: 5 }) + // await rollup.connect(validators[1]).reduceDeposit(initialStake) - await rollupAdmin.pause() - await rollup.connect(validators[1]).reduceDeposit(5) + // await expect( + // rollup.connect(validators[1]).reduceDeposit(initialStake.add(1)) + // ).to.be.revertedWith('TOO_LITTLE_STAKE') - const prevBalance = await validators[1].getBalance() - const prevWithdrawablefunds = await rollup.rollup.withdrawableFunds( - await validators[1].getAddress() - ) + // await rollup + // .connect(validators[1]) + // .addToDeposit(await validators[1].getAddress(), { value: 5 }) - const tx = await rollup.rollup.connect(validators[1]).withdrawStakerFunds() - const receipt = await tx.wait() - const gasPaid = receipt.gasUsed.mul(receipt.effectiveGasPrice) + // await rollupAdmin.pause() + // await rollup.connect(validators[1]).reduceDeposit(5) - const postBalance = await validators[1].getBalance() - const postWithdrawablefunds = await rollup.rollup.withdrawableFunds( - await validators[1].getAddress() - ) + // const prevBalance = await validators[1].getBalance() + // const prevWithdrawablefunds = await rollup.rollup.withdrawableFunds( + // await validators[1].getAddress() + // ) - expect(postWithdrawablefunds).to.equal(0) - expect(postBalance.add(gasPaid)).to.equal( - prevBalance.add(prevWithdrawablefunds) - ) + // const tx = await rollup.rollup.connect(validators[1]).withdrawStakerFunds() + // const receipt = await tx.wait() + // const gasPaid = receipt.gasUsed.mul(receipt.effectiveGasPrice) - // this gets deposit and removes staker - await rollup.rollup - .connect(validators[1]) - .returnOldDeposit(await validators[1].getAddress()) - // all stake is now removed - await rollupAdmin.resume() - }) + // const postBalance = await validators[1].getBalance() + // const postWithdrawablefunds = await rollup.rollup.withdrawableFunds( + // await validators[1].getAddress() + // ) + + // expect(postWithdrawablefunds).to.equal(0) + // expect(postBalance.add(gasPaid)).to.equal( + // prevBalance.add(prevWithdrawablefunds) + // ) + + // // this gets deposit and removes staker + // await rollup.rollup + // .connect(validators[1]) + // .returnOldDeposit(await validators[1].getAddress()) + // // all stake is now removed + // await rollupAdmin.resume() + // }) it('should allow removing zombies', async function () { const zombieCount = ( From 010c37fcbefd4fc044d37ea29fd25e15f8afec8c Mon Sep 17 00:00:00 2001 From: gzeon Date: Tue, 24 Oct 2023 19:47:52 +0800 Subject: [PATCH 04/16] feat: whenNotPausedOrDeprecated --- src/rollup/RollupUserLogic.sol | 30 ++++++++++++++++++++++++++---- 1 file changed, 26 insertions(+), 4 deletions(-) diff --git a/src/rollup/RollupUserLogic.sol b/src/rollup/RollupUserLogic.sol index 0617f363..c3653ede 100644 --- a/src/rollup/RollupUserLogic.sol +++ b/src/rollup/RollupUserLogic.sol @@ -227,6 +227,11 @@ abstract contract AbsRollupUserLogic is stakeOnNode(msg.sender, latestNodeCreated()); } + modifier whenNotPausedOrDeprecated() { + require(!paused() || address(bridge.rollup()) != address(this), "PAUSED_OR_NOT_DEPRECATED"); + _; + } + /** * @notice Refund a staker that is currently staked on or before the latest confirmed node * @dev Since a staker is initially placed in the latest confirmed node, if they don't move it @@ -234,7 +239,12 @@ abstract contract AbsRollupUserLogic is * and move it to the desired node. * @param stakerAddress Address of the staker whose stake is refunded */ - function returnOldDeposit(address stakerAddress) external override onlyValidator { + function returnOldDeposit(address stakerAddress) + external + override + onlyValidator + whenNotPausedOrDeprecated + { require(latestStakedNode(stakerAddress) <= latestConfirmed(), "TOO_RECENT"); requireUnchallengedStaker(stakerAddress); withdrawStaker(stakerAddress); @@ -258,7 +268,7 @@ abstract contract AbsRollupUserLogic is * @notice Reduce the amount staked for the sender (difference between initial amount staked and target is creditted back to the sender). * @param target Target amount of stake for the staker. If this is below the current minimum, it will be set to minimum instead */ - function reduceDeposit(uint256 target) external onlyValidator { + function reduceDeposit(uint256 target) external onlyValidator whenNotPausedOrDeprecated { requireUnchallengedStaker(msg.sender); uint256 currentRequired = currentRequiredStake(); if (target < currentRequired) { @@ -659,7 +669,13 @@ contract RollupUserLogic is AbsRollupUserLogic, IRollupUser { /** * @notice Withdraw uncommitted funds owned by sender from the rollup chain */ - function withdrawStakerFunds() external override onlyValidator returns (uint256) { + function withdrawStakerFunds() + external + override + onlyValidator + whenNotPausedOrDeprecated + returns (uint256) + { uint256 amount = withdrawFunds(msg.sender); // This is safe because it occurs after all checks and effects // solhint-disable-next-line avoid-low-level-calls @@ -731,7 +747,13 @@ contract ERC20RollupUserLogic is AbsRollupUserLogic, IRollupUserERC20 { /** * @notice Withdraw uncommitted funds owned by sender from the rollup chain */ - function withdrawStakerFunds() external override onlyValidator returns (uint256) { + function withdrawStakerFunds() + external + override + onlyValidator + whenNotPausedOrDeprecated + returns (uint256) + { 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"); From 01152be1da813fb60010f0d5dceed0d27fa2d4d7 Mon Sep 17 00:00:00 2001 From: gzeon Date: Tue, 24 Oct 2023 19:57:22 +0800 Subject: [PATCH 05/16] feat: allow updating rollup address in bridge --- src/bridge/AbsBridge.sol | 5 +++++ src/bridge/IBridge.sol | 2 ++ src/mocks/BridgeStub.sol | 4 ++++ src/test-helpers/BridgeTester.sol | 4 ++++ 4 files changed, 15 insertions(+) diff --git a/src/bridge/AbsBridge.sol b/src/bridge/AbsBridge.sol index 88577d2d..29cf6a84 100644 --- a/src/bridge/AbsBridge.sol +++ b/src/bridge/AbsBridge.sol @@ -57,6 +57,11 @@ abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge { address internal constant EMPTY_ACTIVEOUTBOX = address(type(uint160).max); + /// @notice Allows the proxy owner to set the rollup address + function updateRollupAddress(IOwnable _rollup) external onlyDelegated onlyProxyOwner { + rollup = _rollup; + } + modifier onlyRollupOrOwner() { if (msg.sender != address(rollup)) { address rollupOwner = rollup.owner(); diff --git a/src/bridge/IBridge.sol b/src/bridge/IBridge.sol index 49c3c13f..7804f289 100644 --- a/src/bridge/IBridge.sol +++ b/src/bridge/IBridge.sol @@ -97,4 +97,6 @@ interface IBridge { function setDelayedInbox(address inbox, bool enabled) external; function setOutbox(address inbox, bool enabled) external; + + function updateRollupAddress(IOwnable _rollup) external; } diff --git a/src/mocks/BridgeStub.sol b/src/mocks/BridgeStub.sol index 37c994e4..2e2dee05 100644 --- a/src/mocks/BridgeStub.sol +++ b/src/mocks/BridgeStub.sol @@ -45,6 +45,10 @@ contract BridgeStub is IBridge, IEthBridge { revert("NOT_IMPLEMENTED"); } + function updateRollupAddress(IOwnable) external pure { + revert("NOT_IMPLEMENTED"); + } + function enqueueDelayedMessage( uint8 kind, address sender, diff --git a/src/test-helpers/BridgeTester.sol b/src/test-helpers/BridgeTester.sol index bd1fc7da..a739957f 100644 --- a/src/test-helpers/BridgeTester.sol +++ b/src/test-helpers/BridgeTester.sol @@ -74,6 +74,10 @@ contract BridgeTester is Initializable, DelegateCallAware, IBridge, IEthBridge { rollup = rollup_; } + function updateRollupAddress(IOwnable _rollup) external onlyDelegated onlyProxyOwner { + rollup = _rollup; + } + function activeOutbox() public view returns (address) { if (_activeOutbox == EMPTY_ACTIVEOUTBOX) return address(uint160(0)); return _activeOutbox; From 674b0e557f7844a676d2a43124d5b8275b9dbd6c Mon Sep 17 00:00:00 2001 From: gzeon Date: Tue, 24 Oct 2023 20:03:16 +0800 Subject: [PATCH 06/16] feat: allow updating rollup address --- src/bridge/AbsOutbox.sol | 5 +++++ src/bridge/IOutbox.sol | 2 ++ src/bridge/ISequencerInbox.sol | 2 ++ src/bridge/SequencerInbox.sol | 5 +++++ src/rollup/AbsRollupEventInbox.sol | 5 +++++ src/rollup/IRollupEventInbox.sol | 2 ++ src/test-helpers/OutboxWithoutOptTester.sol | 4 ++++ 7 files changed, 25 insertions(+) diff --git a/src/bridge/AbsOutbox.sol b/src/bridge/AbsOutbox.sol index 2b87c726..c9dd3eba 100644 --- a/src/bridge/AbsOutbox.sol +++ b/src/bridge/AbsOutbox.sol @@ -76,6 +76,11 @@ abstract contract AbsOutbox is DelegateCallAware, IOutbox { rollup = address(_bridge.rollup()); } + /// @notice Allows the proxy owner to set the rollup address + function updateRollupAddress() external onlyDelegated onlyProxyOwner { + rollup = address(bridge.rollup()); + } + function updateSendRoot(bytes32 root, bytes32 l2BlockHash) external { if (msg.sender != rollup) revert NotRollup(msg.sender, rollup); roots[root] = l2BlockHash; diff --git a/src/bridge/IOutbox.sol b/src/bridge/IOutbox.sol index 06358170..21bf02f8 100644 --- a/src/bridge/IOutbox.sol +++ b/src/bridge/IOutbox.sol @@ -31,6 +31,8 @@ interface IOutbox { function updateSendRoot(bytes32 sendRoot, bytes32 l2BlockHash) external; + function updateRollupAddress() external; + /// @notice When l2ToL1Sender returns a nonzero address, the message was originated by an L2 account /// When the return value is zero, that means this is a system message /// @dev the l2ToL1Sender behaves as the tx.origin, the msg.sender should be validated to protect against reentrancies diff --git a/src/bridge/ISequencerInbox.sol b/src/bridge/ISequencerInbox.sol index b4fadddb..7d66befc 100644 --- a/src/bridge/ISequencerInbox.sol +++ b/src/bridge/ISequencerInbox.sol @@ -177,4 +177,6 @@ interface ISequencerInbox is IDelayedMessageProvider { // ---------- initializer ---------- function initialize(IBridge bridge_, MaxTimeVariation calldata maxTimeVariation_) external; + + function updateRollupAddress() external; } diff --git a/src/bridge/SequencerInbox.sol b/src/bridge/SequencerInbox.sol index c1cb745f..4f63d716 100644 --- a/src/bridge/SequencerInbox.sol +++ b/src/bridge/SequencerInbox.sol @@ -91,6 +91,11 @@ contract SequencerInbox is DelegateCallAware, GasRefundEnabled, ISequencerInbox maxTimeVariation = maxTimeVariation_; } + /// @notice Allows the proxy owner to set the rollup address + function updateRollupAddress() external onlyDelegated onlyProxyOwner { + rollup = bridge.rollup(); + } + function getTimeBounds() internal view virtual returns (TimeBounds memory) { TimeBounds memory bounds; if (block.timestamp > maxTimeVariation.delaySeconds) { diff --git a/src/rollup/AbsRollupEventInbox.sol b/src/rollup/AbsRollupEventInbox.sol index b2f89fcd..80f905e6 100644 --- a/src/rollup/AbsRollupEventInbox.sol +++ b/src/rollup/AbsRollupEventInbox.sol @@ -37,6 +37,11 @@ abstract contract AbsRollupEventInbox is rollup = address(_bridge.rollup()); } + /// @notice Allows the proxy owner to set the rollup address + function updateRollupAddress() external onlyDelegated onlyProxyOwner { + rollup = address(bridge.rollup()); + } + function rollupInitialized(uint256 chainId, string calldata chainConfig) external override diff --git a/src/rollup/IRollupEventInbox.sol b/src/rollup/IRollupEventInbox.sol index beb1b4ed..2e79f7e6 100644 --- a/src/rollup/IRollupEventInbox.sol +++ b/src/rollup/IRollupEventInbox.sol @@ -13,5 +13,7 @@ interface IRollupEventInbox { function rollup() external view returns (address); + function updateRollupAddress() external; + function rollupInitialized(uint256 chainId, string calldata chainConfig) external; } diff --git a/src/test-helpers/OutboxWithoutOptTester.sol b/src/test-helpers/OutboxWithoutOptTester.sol index 50f378ac..957e7d84 100644 --- a/src/test-helpers/OutboxWithoutOptTester.sol +++ b/src/test-helpers/OutboxWithoutOptTester.sol @@ -54,6 +54,10 @@ contract OutboxWithoutOptTester is DelegateCallAware, IOutbox { emit SendRootUpdated(root, l2BlockHash); } + function updateRollupAddress() external onlyDelegated onlyProxyOwner { + rollup = address(bridge.rollup()); + } + /// @notice When l2ToL1Sender returns a nonzero address, the message was originated by an L2 account /// When the return value is zero, that means this is a system message /// @dev the l2ToL1Sender behaves as the tx.origin, the msg.sender should be validated to protect against reentrancies From 6247b41dde3f18342a2f717ec86c4d7c425893fa Mon Sep 17 00:00:00 2001 From: gzeon Date: Tue, 24 Oct 2023 20:04:53 +0800 Subject: [PATCH 07/16] format: fix --- test/contract/arbRollup.spec.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/test/contract/arbRollup.spec.ts b/test/contract/arbRollup.spec.ts index 24132b29..b7bcb0b2 100644 --- a/test/contract/arbRollup.spec.ts +++ b/test/contract/arbRollup.spec.ts @@ -790,19 +790,23 @@ describe('ArbRollup', () => { }) it('allow force refund staker with pending node', async function () { - await (await rollupAdmin.pause()).wait(); - await (await rollupAdmin.forceRefundStaker([await validators[1].getAddress()])).wait() - await (await rollup.rollup.connect(validators[1]).withdrawStakerFunds()).wait() - await (await rollupAdmin.resume()).wait(); + await (await rollupAdmin.pause()).wait() + await ( + await rollupAdmin.forceRefundStaker([await validators[1].getAddress()]) + ).wait() + await ( + await rollup.rollup.connect(validators[1]).withdrawStakerFunds() + ).wait() + await (await rollupAdmin.resume()).wait() const postWithdrawablefunds = await rollup.rollup.withdrawableFunds( await validators[1].getAddress() ) - expect(postWithdrawablefunds, "withdrawable funds").to.equal(0) + expect(postWithdrawablefunds, 'withdrawable funds').to.equal(0) const stake = await rollup.rollup.amountStaked( await validators[1].getAddress() ) - expect(stake, "amount staked").to.equal(0) + expect(stake, 'amount staked').to.equal(0) }) // it('should add and remove stakes correctly', async function () { From ceed8da1900cea02e518755b2011c024e4cf4041 Mon Sep 17 00:00:00 2001 From: gzeon Date: Tue, 24 Oct 2023 22:06:01 +0800 Subject: [PATCH 08/16] fix: allow rollup admin instead --- src/bridge/AbsBridge.sol | 11 ++++++----- src/bridge/AbsOutbox.sol | 6 ++++-- src/bridge/SequencerInbox.sol | 6 ++++-- src/libraries/Error.sol | 3 +++ src/rollup/AbsRollupEventInbox.sol | 6 ++++-- src/test-helpers/BridgeTester.sol | 2 +- 6 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/bridge/AbsBridge.sol b/src/bridge/AbsBridge.sol index 29cf6a84..51de0dbd 100644 --- a/src/bridge/AbsBridge.sol +++ b/src/bridge/AbsBridge.sol @@ -14,6 +14,7 @@ import { NotSequencerInbox, NotOutbox, InvalidOutboxSet, + InvalidRollupSet, BadSequencerMessageNumber } from "../libraries/Error.sol"; import "./IBridge.sol"; @@ -57,11 +58,6 @@ abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge { address internal constant EMPTY_ACTIVEOUTBOX = address(type(uint160).max); - /// @notice Allows the proxy owner to set the rollup address - function updateRollupAddress(IOwnable _rollup) external onlyDelegated onlyProxyOwner { - rollup = _rollup; - } - modifier onlyRollupOrOwner() { if (msg.sender != address(rollup)) { address rollupOwner = rollup.owner(); @@ -72,6 +68,11 @@ abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge { _; } + /// @notice Allows the rollup owner to set another rollup address + function updateRollupAddress(IOwnable _rollup) external onlyRollupOrOwner { + rollup = _rollup; + } + /// @dev returns the address of current active Outbox, or zero if no outbox is active function activeOutbox() public view returns (address) { address outbox = _activeOutbox; diff --git a/src/bridge/AbsOutbox.sol b/src/bridge/AbsOutbox.sol index c9dd3eba..f49e092b 100644 --- a/src/bridge/AbsOutbox.sol +++ b/src/bridge/AbsOutbox.sol @@ -76,8 +76,10 @@ abstract contract AbsOutbox is DelegateCallAware, IOutbox { rollup = address(_bridge.rollup()); } - /// @notice Allows the proxy owner to set the rollup address - function updateRollupAddress() external onlyDelegated onlyProxyOwner { + /// @notice Allows the rollup owner to sync the rollup address + function updateRollupAddress() external { + if (msg.sender != IOwnable(rollup).owner()) + revert NotOwner(msg.sender, IOwnable(rollup).owner()); rollup = address(bridge.rollup()); } diff --git a/src/bridge/SequencerInbox.sol b/src/bridge/SequencerInbox.sol index 4f63d716..921122f3 100644 --- a/src/bridge/SequencerInbox.sol +++ b/src/bridge/SequencerInbox.sol @@ -91,8 +91,10 @@ contract SequencerInbox is DelegateCallAware, GasRefundEnabled, ISequencerInbox maxTimeVariation = maxTimeVariation_; } - /// @notice Allows the proxy owner to set the rollup address - function updateRollupAddress() external onlyDelegated onlyProxyOwner { + /// @notice Allows the rollup owner to sync the rollup address + function updateRollupAddress() external { + if (msg.sender != IOwnable(rollup).owner()) + revert NotOwner(msg.sender, IOwnable(rollup).owner()); rollup = bridge.rollup(); } diff --git a/src/libraries/Error.sol b/src/libraries/Error.sol index beb0afc8..d70f92d4 100644 --- a/src/libraries/Error.sol +++ b/src/libraries/Error.sol @@ -172,3 +172,6 @@ error AlreadyValidDASKeyset(bytes32); /// @dev Tried to use or invalidate an already invalid Data Availability Service keyset error NoSuchKeyset(bytes32); + +/// @dev Invalid rollup address is set +error InvalidRollupSet(address); diff --git a/src/rollup/AbsRollupEventInbox.sol b/src/rollup/AbsRollupEventInbox.sol index 80f905e6..6fa42ea3 100644 --- a/src/rollup/AbsRollupEventInbox.sol +++ b/src/rollup/AbsRollupEventInbox.sol @@ -37,8 +37,10 @@ abstract contract AbsRollupEventInbox is rollup = address(_bridge.rollup()); } - /// @notice Allows the proxy owner to set the rollup address - function updateRollupAddress() external onlyDelegated onlyProxyOwner { + /// @notice Allows the rollup owner to sync the rollup address + function updateRollupAddress() external { + if (msg.sender != IOwnable(rollup).owner()) + revert NotOwner(msg.sender, IOwnable(rollup).owner()); rollup = address(bridge.rollup()); } diff --git a/src/test-helpers/BridgeTester.sol b/src/test-helpers/BridgeTester.sol index a739957f..0bcbe00f 100644 --- a/src/test-helpers/BridgeTester.sol +++ b/src/test-helpers/BridgeTester.sol @@ -74,7 +74,7 @@ contract BridgeTester is Initializable, DelegateCallAware, IBridge, IEthBridge { rollup = rollup_; } - function updateRollupAddress(IOwnable _rollup) external onlyDelegated onlyProxyOwner { + function updateRollupAddress(IOwnable _rollup) external { rollup = _rollup; } From baade430eda5cd8a08ddc32534049dd1ea7dfa4d Mon Sep 17 00:00:00 2001 From: gzeon Date: Tue, 24 Oct 2023 22:06:10 +0800 Subject: [PATCH 09/16] test: fixes --- test/contract/arbRollup.spec.ts | 133 +++++++++++++++++++------------- 1 file changed, 78 insertions(+), 55 deletions(-) diff --git a/test/contract/arbRollup.spec.ts b/test/contract/arbRollup.spec.ts index b7bcb0b2..0dcf03d8 100644 --- a/test/contract/arbRollup.spec.ts +++ b/test/contract/arbRollup.spec.ts @@ -46,6 +46,7 @@ import { RollupUserLogic__factory, SequencerInbox, SequencerInbox__factory, + Bridge, } from '../../build/types' import { abi as UpgradeExecutorABI, @@ -86,6 +87,7 @@ const wasmModuleRoot = let rollup: RollupContract let rollupUser: RollupUserLogic let rollupAdmin: RollupAdminLogic +let bridge: Bridge let accounts: Signer[] let validators: Signer[] let sequencerInbox: SequencerInbox @@ -93,6 +95,7 @@ let admin: Signer let sequencer: Signer let challengeManager: ChallengeManager let upgradeExecutor: string +let adminproxy: string async function getDefaultConfig( _confirmPeriodBlocks = confirmationPeriodBlocks @@ -275,6 +278,7 @@ const setup = async () => { await val1.getAddress(), await val2.getAddress(), await val3.getAddress(), + await val4.getAddress(), ], maxDataSize: 117964, nativeToken: ethers.constants.AddressZero, @@ -298,6 +302,7 @@ const setup = async () => { const rollupUser = rollupUserLogicFac .attach(rollupCreatedEvent.rollupAddress) .connect(user) + const bridge = ethBridgeFac.attach(rollupCreatedEvent.bridge).connect(user) sequencerInbox = ( (await ethers.getContractFactory( @@ -328,8 +333,9 @@ const setup = async () => { sequencerInbox: rollupCreatedEvent.sequencerInbox, delayedBridge: rollupCreatedEvent.bridge, delayedInbox: rollupCreatedEvent.inboxAddress, - bridge: rollupCreatedEvent.bridge, + bridge, upgradeExecutorAddress: rollupCreatedEvent.upgradeExecutor, + adminproxy: rollupCreatedEvent.adminProxy, } } @@ -509,15 +515,19 @@ describe('ArbRollup', () => { const { rollupAdmin: rollupAdminContract, rollupUser: rollupUserContract, + bridge: bridgeContract, admin: adminI, validators: validatorsI, upgradeExecutorAddress, + adminproxy: adminproxyAddress, } = await setup() rollupAdmin = rollupAdminContract rollupUser = rollupUserContract + bridge = bridgeContract admin = adminI validators = validatorsI upgradeExecutor = upgradeExecutorAddress + adminproxy = adminproxyAddress rollup = new RollupContract(rollupUser.connect(validators[0])) }) @@ -580,6 +590,9 @@ describe('ArbRollup', () => { await rollupUser .connect(validators[2]) .newStakeOnExistingNode(1, prevNode.nodeHash, { value: stake }) + await rollupUser + .connect(validators[3]) + .newStakeOnExistingNode(1, prevNode.nodeHash, { value: stake }) }) it('should move stake to a new node', async function () { @@ -790,88 +803,98 @@ describe('ArbRollup', () => { }) it('allow force refund staker with pending node', async function () { - await (await rollupAdmin.pause()).wait() + await rollupAdmin.connect(await impersonateAccount(upgradeExecutor)).pause() await ( - await rollupAdmin.forceRefundStaker([await validators[1].getAddress()]) + await rollupAdmin + .connect(await impersonateAccount(upgradeExecutor)) + .forceRefundStaker([await validators[3].getAddress()]) ).wait() + // staker can only withdraw if rollup address changed when paused + await bridge + .connect(await impersonateAccount(rollup.rollup.address)) + .updateRollupAddress(ethers.constants.AddressZero) await ( - await rollup.rollup.connect(validators[1]).withdrawStakerFunds() + await rollup.rollup.connect(validators[3]).withdrawStakerFunds() ).wait() - await (await rollupAdmin.resume()).wait() + await rollupAdmin + .connect(await impersonateAccount(upgradeExecutor)) + .resume() + // restore rollup address + await bridge + .connect(await impersonateAccount(ethers.constants.AddressZero)) + .updateRollupAddress(rollupUser.address) const postWithdrawablefunds = await rollup.rollup.withdrawableFunds( - await validators[1].getAddress() + await validators[3].getAddress() ) expect(postWithdrawablefunds, 'withdrawable funds').to.equal(0) const stake = await rollup.rollup.amountStaked( - await validators[1].getAddress() + await validators[3].getAddress() ) expect(stake, 'amount staked').to.equal(0) }) - // it('should add and remove stakes correctly', async function () { - // /* - // RollupUser functions that alter stake and their respective Core logic + it('should add and remove stakes correctly', async function () { + /* + RollupUser functions that alter stake and their respective Core logic - // user: newStake - // core: createNewStake + user: newStake + core: createNewStake - // user: addToDeposit - // core: increaseStakeBy + user: addToDeposit + core: increaseStakeBy - // user: reduceDeposit - // core: reduceStakeTo + user: reduceDeposit + core: reduceStakeTo - // user: returnOldDeposit - // core: withdrawStaker + user: returnOldDeposit + core: withdrawStaker - // user: withdrawStakerFunds - // core: withdrawFunds - // */ + user: withdrawStakerFunds + core: withdrawFunds + */ - // const initialStake = await rollup.rollup.amountStaked( - // await validators[1].getAddress() - // ) + const initialStake = await rollup.rollup.amountStaked( + await validators[1].getAddress() + ) - // await rollup.connect(validators[1]).reduceDeposit(initialStake) + await rollup.connect(validators[1]).reduceDeposit(initialStake) - // await expect( - // rollup.connect(validators[1]).reduceDeposit(initialStake.add(1)) - // ).to.be.revertedWith('TOO_LITTLE_STAKE') + await expect( + rollup.connect(validators[1]).reduceDeposit(initialStake.add(1)) + ).to.be.revertedWith('TOO_LITTLE_STAKE') - // await rollup - // .connect(validators[1]) - // .addToDeposit(await validators[1].getAddress(), { value: 5 }) + await rollup + .connect(validators[1]) + .addToDeposit(await validators[1].getAddress(), { value: 5 }) - // await rollupAdmin.pause() - // await rollup.connect(validators[1]).reduceDeposit(5) + await rollup.connect(validators[1]).reduceDeposit(5) - // const prevBalance = await validators[1].getBalance() - // const prevWithdrawablefunds = await rollup.rollup.withdrawableFunds( - // await validators[1].getAddress() - // ) + const prevBalance = await validators[1].getBalance() + const prevWithdrawablefunds = await rollup.rollup.withdrawableFunds( + await validators[1].getAddress() + ) - // const tx = await rollup.rollup.connect(validators[1]).withdrawStakerFunds() - // const receipt = await tx.wait() - // const gasPaid = receipt.gasUsed.mul(receipt.effectiveGasPrice) + const tx = await rollup.rollup.connect(validators[1]).withdrawStakerFunds() + const receipt = await tx.wait() + const gasPaid = receipt.gasUsed.mul(receipt.effectiveGasPrice) - // const postBalance = await validators[1].getBalance() - // const postWithdrawablefunds = await rollup.rollup.withdrawableFunds( - // await validators[1].getAddress() - // ) + const postBalance = await validators[1].getBalance() + const postWithdrawablefunds = await rollup.rollup.withdrawableFunds( + await validators[1].getAddress() + ) - // expect(postWithdrawablefunds).to.equal(0) - // expect(postBalance.add(gasPaid)).to.equal( - // prevBalance.add(prevWithdrawablefunds) - // ) + expect(postWithdrawablefunds).to.equal(0) + expect(postBalance.add(gasPaid)).to.equal( + prevBalance.add(prevWithdrawablefunds) + ) - // // this gets deposit and removes staker - // await rollup.rollup - // .connect(validators[1]) - // .returnOldDeposit(await validators[1].getAddress()) - // // all stake is now removed - // await rollupAdmin.resume() - // }) + // this gets deposit and removes staker + await rollup.rollup + .connect(validators[1]) + .returnOldDeposit(await validators[1].getAddress()) + // all stake is now removed + }) it('should allow removing zombies', async function () { const zombieCount = ( From 5c723cb89af79fc9ffeb5ae4388d64418a0753fa Mon Sep 17 00:00:00 2001 From: gzeon Date: Tue, 24 Oct 2023 22:23:09 +0800 Subject: [PATCH 10/16] chore: remove unused error --- src/bridge/AbsBridge.sol | 1 - src/libraries/Error.sol | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/bridge/AbsBridge.sol b/src/bridge/AbsBridge.sol index 51de0dbd..e4e0ae57 100644 --- a/src/bridge/AbsBridge.sol +++ b/src/bridge/AbsBridge.sol @@ -14,7 +14,6 @@ import { NotSequencerInbox, NotOutbox, InvalidOutboxSet, - InvalidRollupSet, BadSequencerMessageNumber } from "../libraries/Error.sol"; import "./IBridge.sol"; diff --git a/src/libraries/Error.sol b/src/libraries/Error.sol index d70f92d4..e4776d80 100644 --- a/src/libraries/Error.sol +++ b/src/libraries/Error.sol @@ -7,9 +7,12 @@ pragma solidity ^0.8.4; /// @dev Init was already called error AlreadyInit(); -/// Init was called with param set to zero that must be nonzero +/// @dev Init was called with param set to zero that must be nonzero error HadZeroInit(); +/// @dev Thrown when post upgrade init validation fails +error BadPostUpgradeInit(); + /// @dev Thrown when non owner tries to access an only-owner function /// @param sender The msg.sender who is not the owner /// @param owner The owner address @@ -172,6 +175,3 @@ error AlreadyValidDASKeyset(bytes32); /// @dev Tried to use or invalidate an already invalid Data Availability Service keyset error NoSuchKeyset(bytes32); - -/// @dev Invalid rollup address is set -error InvalidRollupSet(address); From fa10996cc89fc0b0aad80c915283f515e296db5e Mon Sep 17 00:00:00 2001 From: gzeon Date: Tue, 24 Oct 2023 22:25:39 +0800 Subject: [PATCH 11/16] docs: improve revert string --- src/rollup/RollupUserLogic.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rollup/RollupUserLogic.sol b/src/rollup/RollupUserLogic.sol index c3653ede..4f565f21 100644 --- a/src/rollup/RollupUserLogic.sol +++ b/src/rollup/RollupUserLogic.sol @@ -228,7 +228,7 @@ abstract contract AbsRollupUserLogic is } modifier whenNotPausedOrDeprecated() { - require(!paused() || address(bridge.rollup()) != address(this), "PAUSED_OR_NOT_DEPRECATED"); + require(!paused() || address(bridge.rollup()) != address(this), "PAUSED_AND_ACTIVE"); _; } From 4d2e1de0d7a04dc56cd24da87139e26ff09979bd Mon Sep 17 00:00:00 2001 From: gzeon Date: Tue, 24 Oct 2023 22:28:08 +0800 Subject: [PATCH 12/16] test: PAUSED_AND_ACTIVE --- test/contract/arbRollup.spec.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/contract/arbRollup.spec.ts b/test/contract/arbRollup.spec.ts index 0dcf03d8..ac836d1d 100644 --- a/test/contract/arbRollup.spec.ts +++ b/test/contract/arbRollup.spec.ts @@ -809,16 +809,22 @@ describe('ArbRollup', () => { .connect(await impersonateAccount(upgradeExecutor)) .forceRefundStaker([await validators[3].getAddress()]) ).wait() + + await expect( + rollup.rollup.connect(validators[3]).withdrawStakerFunds() + ).to.be.revertedWith('PAUSED_AND_ACTIVE') // staker can only withdraw if rollup address changed when paused await bridge .connect(await impersonateAccount(rollup.rollup.address)) .updateRollupAddress(ethers.constants.AddressZero) + await ( await rollup.rollup.connect(validators[3]).withdrawStakerFunds() ).wait() await rollupAdmin .connect(await impersonateAccount(upgradeExecutor)) .resume() + // restore rollup address await bridge .connect(await impersonateAccount(ethers.constants.AddressZero)) From 1075cc82692849b603d3d172f2a92bb98fece766 Mon Sep 17 00:00:00 2001 From: gzeon Date: Tue, 24 Oct 2023 22:49:08 +0800 Subject: [PATCH 13/16] chore: remove unrelated error --- src/libraries/Error.sol | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/libraries/Error.sol b/src/libraries/Error.sol index e4776d80..2d2461b3 100644 --- a/src/libraries/Error.sol +++ b/src/libraries/Error.sol @@ -10,9 +10,6 @@ error AlreadyInit(); /// @dev Init was called with param set to zero that must be nonzero error HadZeroInit(); -/// @dev Thrown when post upgrade init validation fails -error BadPostUpgradeInit(); - /// @dev Thrown when non owner tries to access an only-owner function /// @param sender The msg.sender who is not the owner /// @param owner The owner address From ca6e8ef21d451b12b2d150dd9c00cc92ca6475cb Mon Sep 17 00:00:00 2001 From: gzeon Date: Tue, 24 Oct 2023 23:30:51 +0800 Subject: [PATCH 14/16] feat: add RollupUpdated event --- src/bridge/AbsBridge.sol | 1 + src/bridge/IBridge.sol | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/bridge/AbsBridge.sol b/src/bridge/AbsBridge.sol index e4e0ae57..bafd56f9 100644 --- a/src/bridge/AbsBridge.sol +++ b/src/bridge/AbsBridge.sol @@ -70,6 +70,7 @@ abstract contract AbsBridge is Initializable, DelegateCallAware, IBridge { /// @notice Allows the rollup owner to set another rollup address function updateRollupAddress(IOwnable _rollup) external onlyRollupOrOwner { rollup = _rollup; + emit RollupUpdated(address(_rollup)); } /// @dev returns the address of current active Outbox, or zero if no outbox is active diff --git a/src/bridge/IBridge.sol b/src/bridge/IBridge.sol index 7804f289..22388b4b 100644 --- a/src/bridge/IBridge.sol +++ b/src/bridge/IBridge.sol @@ -32,6 +32,8 @@ interface IBridge { event SequencerInboxUpdated(address newSequencerInbox); + event RollupUpdated(address rollup); + function allowedDelayedInboxList(uint256) external returns (address); function allowedOutboxList(uint256) external returns (address); From e8aac68c2fb19a8806a8c3dc398fdba5a39dfcca Mon Sep 17 00:00:00 2001 From: gzeon Date: Tue, 24 Oct 2023 23:36:17 +0800 Subject: [PATCH 15/16] feat: throw RollupNotChanged --- src/bridge/AbsOutbox.sol | 7 +++++-- src/bridge/SequencerInbox.sol | 7 +++++-- src/libraries/Error.sol | 3 +++ src/rollup/AbsRollupEventInbox.sol | 6 ++++-- 4 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/bridge/AbsOutbox.sol b/src/bridge/AbsOutbox.sol index 126f6062..4dabf9ff 100644 --- a/src/bridge/AbsOutbox.sol +++ b/src/bridge/AbsOutbox.sol @@ -13,7 +13,8 @@ import { AlreadySpent, BridgeCallFailed, HadZeroInit, - BadPostUpgradeInit + BadPostUpgradeInit, + RollupNotChanged } from "../libraries/Error.sol"; import "./IBridge.sol"; import "./IOutbox.sol"; @@ -94,7 +95,9 @@ abstract contract AbsOutbox is DelegateCallAware, IOutbox { function updateRollupAddress() external { if (msg.sender != IOwnable(rollup).owner()) revert NotOwner(msg.sender, IOwnable(rollup).owner()); - rollup = address(bridge.rollup()); + address newRollup = address(bridge.rollup()); + if (rollup == newRollup) revert RollupNotChanged(); + rollup = newRollup; } function updateSendRoot(bytes32 root, bytes32 l2BlockHash) external { diff --git a/src/bridge/SequencerInbox.sol b/src/bridge/SequencerInbox.sol index 921122f3..51d511e2 100644 --- a/src/bridge/SequencerInbox.sol +++ b/src/bridge/SequencerInbox.sol @@ -20,7 +20,8 @@ import { DataNotAuthenticated, AlreadyValidDASKeyset, NoSuchKeyset, - NotForked + NotForked, + RollupNotChanged } from "../libraries/Error.sol"; import "./IBridge.sol"; import "./IInboxBase.sol"; @@ -95,7 +96,9 @@ contract SequencerInbox is DelegateCallAware, GasRefundEnabled, ISequencerInbox function updateRollupAddress() external { if (msg.sender != IOwnable(rollup).owner()) revert NotOwner(msg.sender, IOwnable(rollup).owner()); - rollup = bridge.rollup(); + IOwnable newRollup = bridge.rollup(); + if (rollup == newRollup) revert RollupNotChanged(); + rollup = newRollup; } function getTimeBounds() internal view virtual returns (TimeBounds memory) { diff --git a/src/libraries/Error.sol b/src/libraries/Error.sol index e4776d80..fd6f3255 100644 --- a/src/libraries/Error.sol +++ b/src/libraries/Error.sol @@ -175,3 +175,6 @@ error AlreadyValidDASKeyset(bytes32); /// @dev Tried to use or invalidate an already invalid Data Availability Service keyset error NoSuchKeyset(bytes32); + +/// @dev Thrown when rollup is not updated with updateRollupAddress +error RollupNotChanged(); diff --git a/src/rollup/AbsRollupEventInbox.sol b/src/rollup/AbsRollupEventInbox.sol index 6fa42ea3..c0866d9e 100644 --- a/src/rollup/AbsRollupEventInbox.sol +++ b/src/rollup/AbsRollupEventInbox.sol @@ -12,7 +12,7 @@ import "../libraries/ArbitrumChecker.sol"; import "../bridge/IDelayedMessageProvider.sol"; import "../libraries/DelegateCallAware.sol"; import {INITIALIZATION_MSG_TYPE} from "../libraries/MessageTypes.sol"; -import {AlreadyInit, HadZeroInit} from "../libraries/Error.sol"; +import {AlreadyInit, HadZeroInit, RollupNotChanged} from "../libraries/Error.sol"; /** * @title The inbox for rollup protocol events @@ -41,7 +41,9 @@ abstract contract AbsRollupEventInbox is function updateRollupAddress() external { if (msg.sender != IOwnable(rollup).owner()) revert NotOwner(msg.sender, IOwnable(rollup).owner()); - rollup = address(bridge.rollup()); + address newRollup = address(bridge.rollup()); + if (rollup == newRollup) revert RollupNotChanged(); + rollup = newRollup; } function rollupInitialized(uint256 chainId, string calldata chainConfig) From 0c248ffe7994f4ba8c2167fed31536571b96fdf5 Mon Sep 17 00:00:00 2001 From: gzeon Date: Tue, 24 Oct 2023 23:37:59 +0800 Subject: [PATCH 16/16] format: move modifier to top --- src/rollup/RollupUserLogic.sol | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/rollup/RollupUserLogic.sol b/src/rollup/RollupUserLogic.sol index 4f565f21..9807f201 100644 --- a/src/rollup/RollupUserLogic.sol +++ b/src/rollup/RollupUserLogic.sol @@ -27,6 +27,11 @@ abstract contract AbsRollupUserLogic is _; } + modifier whenNotPausedOrDeprecated() { + require(!paused() || address(bridge.rollup()) != address(this), "PAUSED_AND_ACTIVE"); + _; + } + uint256 internal immutable deployTimeChainId = block.chainid; function _chainIdChanged() internal view returns (bool) { @@ -227,11 +232,6 @@ abstract contract AbsRollupUserLogic is stakeOnNode(msg.sender, latestNodeCreated()); } - modifier whenNotPausedOrDeprecated() { - require(!paused() || address(bridge.rollup()) != address(this), "PAUSED_AND_ACTIVE"); - _; - } - /** * @notice Refund a staker that is currently staked on or before the latest confirmed node * @dev Since a staker is initially placed in the latest confirmed node, if they don't move it