fix(protocol): fix an issue in DelegateOwner then refactor the code (#…

…17633)

Co-authored-by: dantaik <dantaik@users.noreply.github.com>

packages/protocol/contract_layout.md

@@ -102,10 +102,10 @@
 | __paused       | uint8       | 201  | 1      | 1     | contracts/L2/DelegateOwner.sol:DelegateOwner |
 | lastUnpausedAt | uint64      | 201  | 2      | 8     | contracts/L2/DelegateOwner.sol:DelegateOwner |
 | __gap          | uint256[49] | 202  | 0      | 1568  | contracts/L2/DelegateOwner.sol:DelegateOwner |
-| l1ChainId      | uint64      | 251  | 0      | 8     | contracts/L2/DelegateOwner.sol:DelegateOwner |
+| remoteChainId  | uint64      | 251  | 0      | 8     | contracts/L2/DelegateOwner.sol:DelegateOwner |
 | admin          | address     | 251  | 8      | 20    | contracts/L2/DelegateOwner.sol:DelegateOwner |
 | nextTxId       | uint64      | 252  | 0      | 8     | contracts/L2/DelegateOwner.sol:DelegateOwner |
-| realOwner      | address     | 252  | 8      | 20    | contracts/L2/DelegateOwner.sol:DelegateOwner |
+| remoteOwner    | address     | 252  | 8      | 20    | contracts/L2/DelegateOwner.sol:DelegateOwner |
 | __gap          | uint256[48] | 253  | 0      | 1536  | contracts/L2/DelegateOwner.sol:DelegateOwner |
 
 ## GuardianProver

packages/protocol/contracts/L2/DelegateOwner.sol

@@ -14,7 +14,7 @@ import "../bridge/IBridge.sol";
 /// @custom:security-contact security@taiko.xyz
 contract DelegateOwner is EssentialContract, IMessageInvocable {
     /// @notice The owner chain ID.
-    uint64 public l1ChainId; // slot 1
+    uint64 public remoteChainId; // slot 1
 
     /// @notice The admin who can directly call `invokeCall`.
     address public admin;
@@ -23,7 +23,7 @@ contract DelegateOwner is EssentialContract, IMessageInvocable {
     uint64 public nextTxId; // slot 2
 
     /// @notice The real owner on L1, supposedly the DAO.
-    address public realOwner;
+    address public remoteOwner;
 
     uint256[48] private __gap;
 
@@ -50,20 +50,26 @@ contract DelegateOwner is EssentialContract, IMessageInvocable {
 
     error DO_DRYRUN_SUCCEEDED();
     error DO_INVALID_PARAM();
+    error DO_INVALID_SENDER();
     error DO_INVALID_TARGET();
     error DO_INVALID_TX_ID();
     error DO_PERMISSION_DENIED();
 
+    modifier onlyAdminOrRemoteOwner() {
+        if (!_isAdminOrRemoteOwner(msg.sender)) revert DO_PERMISSION_DENIED();
+        _;
+    }
+
     /// @notice Initializes the contract.
-    /// @param _realOwner The real owner on L1 that can send a cross-chain message to invoke
+    /// @param _remoteOwner The real owner on L1 that can send a cross-chain message to invoke
     /// `onMessageInvocation`.
-    /// @param _l1ChainId The L1 chain's ID.
+    /// @param _remoteChainId The L1 chain's ID.
     /// @param _addressManager The address of the {AddressManager} contract.
     /// @param _admin The admin address.
     function init(
-        address _realOwner,
+        address _remoteOwner,
         address _addressManager,
-        uint64 _l1ChainId,
+        uint64 _remoteChainId,
         address _admin
     )
         external
@@ -72,63 +78,49 @@ contract DelegateOwner is EssentialContract, IMessageInvocable {
         // This contract's owner will be itself.
         __Essential_init(address(this), _addressManager);
 
-        if (_realOwner == address(0) || _l1ChainId == 0 || _l1ChainId == block.chainid) {
+        if (_remoteOwner == address(0) || _remoteChainId == 0 || _remoteChainId == block.chainid) {
             revert DO_INVALID_PARAM();
         }
 
-        l1ChainId = _l1ChainId;
-        realOwner = _realOwner;
+        remoteChainId = _remoteChainId;
+        remoteOwner = _remoteOwner;
         admin = _admin;
     }
 
     /// @inheritdoc IMessageInvocable
-    function onMessageInvocation(bytes calldata _data)
-        external
-        payable
-        onlyFromNamed(LibStrings.B_BRIDGE)
-    {
-        IBridge.Context memory ctx = IBridge(msg.sender).context();
-        if (ctx.srcChainId != l1ChainId || ctx.from != realOwner) {
-            revert DO_PERMISSION_DENIED();
-        }
+    function onMessageInvocation(bytes calldata _data) external payable onlyAdminOrRemoteOwner {
         _invokeCall(_data, true);
     }
 
-    /// @dev Invokes a call by the admin
-    /// @param _data The data for this contract to interpret.
-    function invokeCall(bytes calldata _data) external {
-        if (msg.sender != admin) revert DO_PERMISSION_DENIED();
-        _invokeCall(_data, true);
+    /// @notice Dryruns a message invocation but always revert.
+    /// If this tx is reverted with DO_TRY_RUN_SUCCEEDED, the try run is successful.
+    /// Note that this function shall not be used in transaction and is designed for offchain
+    /// simulation only.
+    function dryrunInvocation(bytes calldata _data) external payable {
+        _invokeCall(_data, false);
+        revert DO_DRYRUN_SUCCEEDED();
     }
 
     /// @dev Updates the admin address.
     /// @param _admin The new admin address.
-    function setAdmin(address _admin) external {
-        if (msg.sender != owner() && msg.sender != admin) revert DO_PERMISSION_DENIED();
-        if (admin == _admin) revert DO_INVALID_PARAM();
+    function setAdmin(address _admin) external nonReentrant onlyOwner {
+        if (_admin == admin || _admin == address(this)) revert DO_INVALID_PARAM();
 
         emit AdminUpdated(admin, _admin);
         admin = _admin;
     }
 
-    /// @notice Dryruns a message invocation but always revert.
-    /// If this tx is reverted with DO_TRY_RUN_SUCCEEDED, the try run is successful.
-    /// Note that this function shall not be used in transaction and is designed for offchain
-    /// simulation only.
-    function dryrunCall(bytes calldata _data) external payable {
-        _invokeCall(_data, false);
-        revert DO_DRYRUN_SUCCEEDED();
-    }
-
-    function acceptOwnership(address target) external {
-        Ownable2StepUpgradeable(target).acceptOwnership();
+    /// @dev Accepts contract ownership
+    /// @param _target Target addresses.
+    function acceptOwnership(address _target) external nonReentrant onlyOwner {
+        Ownable2StepUpgradeable(_target).acceptOwnership();
     }
 
     function transferOwnership(address) public pure override notImplemented { }
 
     function _authorizePause(address, bool) internal pure override notImplemented { }
 
-    function _invokeCall(bytes calldata _data, bool _verifyTxId) internal {
+    function _invokeCall(bytes calldata _data, bool _verifyTxId) private {
         Call memory call = abi.decode(_data, (Call));
 
         if (_verifyTxId && call.txId != nextTxId++) revert DO_INVALID_TX_ID();
@@ -143,4 +135,12 @@ contract DelegateOwner is EssentialContract, IMessageInvocable {
         if (!success) LibBytes.revertWithExtractedError(result);
         emit MessageInvoked(call.txId, call.target, call.isDelegateCall, bytes4(call.txdata));
     }
+
+    function _isAdminOrRemoteOwner(address _sender) private view returns (bool) {
+        if (_sender == admin) return true;
+        if (_sender != resolve(LibStrings.B_BRIDGE, false)) return false;
+
+        IBridge.Context memory ctx = IBridge(_sender).context();
+        return ctx.srcChainId == remoteChainId && ctx.from == remoteOwner;
+    }
 }

packages/protocol/deployments/mainnet-contract-logs-L2.md

@@ -113,6 +113,16 @@
 - todo:
   - change owner to DelegateOwner
 
+#### delegate_owner
+
+- proxy: `0x904aa0aC002532f1410457484893107757683F53`
+- impl: `0x9F0C40A474E0FB6b27D71c43Aff840B9c42f0C44`
+- admin: `0x8F13E3a9dFf52e282884aA70eAe93F57DD601298`
+- remoteOwner: `0x8F13E3a9dFf52e282884aA70eAe93F57DD601298`
+- todo:
+  - test various use cases
+  - transfer remote owner to `admin.taiko.eth`
+
 ## Rollup Specific
 
 #### rollup_address_manager (ram)

packages/protocol/script/DeployL2DelegateOwner.s.sol

@@ -17,19 +17,10 @@ contract DeployL2DelegateOwner is DeployCapability {
     }
 
     function run() external broadcast {
-        // Deploy the QuotaManager contract on Ethereum
-        // 0x08c82ab90f86bf8d98440b96754a67411d656130
-        address delegateowner = deployProxy({
+        deployProxy({
             name: "delegate_owner",
-            impl: address(new DelegateOwner()), // 0xdaf15cfa36c2188e3e0f4fb15a80e476e5e2ceb9
+            impl: address(new DelegateOwner()),
             data: abi.encodeCall(DelegateOwner.init, (l1Owner, l2Sam, 1, l2Admin))
         });
-
-        // 0xf4707c2821b3067bdf9c4d48eb133851ff3e7ea7
-        deployProxy({
-            name: "test_address_am",
-            impl: address(new AddressManager()), // 0x66489c2932a906ea7971eeb0a7379593ea32eb79
-            data: abi.encodeCall(AddressManager.init, (delegateowner))
-        });
     }
 }

packages/protocol/test/L2/DelegateOwner.t.sol

@@ -95,7 +95,7 @@ contract TestDelegateOwner is TaikoTest {
         );
 
         vm.expectRevert(DelegateOwner.DO_DRYRUN_SUCCEEDED.selector);
-        delegateOwner.dryrunCall(data);
+        delegateOwner.dryrunInvocation(data);
 
         IBridge.Message memory message;
         message.from = remoteOwner;
@@ -128,7 +128,7 @@ contract TestDelegateOwner is TaikoTest {
         );
 
         vm.expectRevert(DelegateOwner.DO_DRYRUN_SUCCEEDED.selector);
-        delegateOwner.dryrunCall(data);
+        delegateOwner.dryrunInvocation(data);
 
         IBridge.Message memory message;
         message.from = remoteOwner;
@@ -169,7 +169,7 @@ contract TestDelegateOwner is TaikoTest {
             })
         );
 
-        TestMulticall3.Call3[] memory calls = new TestMulticall3.Call3[](3);
+        TestMulticall3.Call3[] memory calls = new TestMulticall3.Call3[](4);
         calls[0].target = address(target1);
         calls[0].allowFailure = false;
         calls[0].callData = abi.encodeCall(EssentialContract.pause, ());
@@ -182,6 +182,10 @@ contract TestDelegateOwner is TaikoTest {
         calls[2].allowFailure = false;
         calls[2].callData = abi.encodeCall(UUPSUpgradeable.upgradeTo, (delegateOwnerImpl2));
 
+        calls[3].target = address(delegateOwner);
+        calls[3].allowFailure = false;
+        calls[3].callData = abi.encodeCall(DelegateOwner.setAdmin, (David));
+
         bytes memory data = abi.encode(
             DelegateOwner.Call(
                 uint64(0),
@@ -192,7 +196,7 @@ contract TestDelegateOwner is TaikoTest {
         );
 
         vm.expectRevert(DelegateOwner.DO_DRYRUN_SUCCEEDED.selector);
-        delegateOwner.dryrunCall(data);
+        delegateOwner.dryrunInvocation(data);
 
         IBridge.Message memory message;
         message.from = remoteOwner;
@@ -212,61 +216,6 @@ contract TestDelegateOwner is TaikoTest {
         assertTrue(target1.paused());
         assertEq(target2.impl(), impl2);
         assertEq(delegateOwner.impl(), delegateOwnerImpl2);
-    }
-
-    function test_delegate_owner_update_admin() public {
-        assertEq(delegateOwner.admin(), address(0));
-        bytes memory data = abi.encode(
-            DelegateOwner.Call(
-                uint64(0),
-                address(delegateOwner),
-                false, // CALL
-                abi.encodeCall(DelegateOwner.setAdmin, (David))
-            )
-        );
-
-        vm.expectRevert(DelegateOwner.DO_DRYRUN_SUCCEEDED.selector);
-        delegateOwner.dryrunCall(data);
-
-        IBridge.Message memory message;
-        message.from = remoteOwner;
-        message.destChainId = uint64(block.chainid);
-        message.srcChainId = remoteChainId;
-        message.destOwner = Bob;
-        message.data = abi.encodeCall(DelegateOwner.onMessageInvocation, (data));
-        message.to = address(delegateOwner);
-
-        vm.prank(Bob);
-        bridge.processMessage(message, "");
-
-        bytes32 hash = bridge.hashMessage(message);
-        assertTrue(bridge.messageStatus(hash) == IBridge.Status.DONE);
-
-        assertEq(delegateOwner.nextTxId(), 1);
         assertEq(delegateOwner.admin(), David);
-
-        // David is now the security council
-        data = abi.encode(
-            DelegateOwner.Call(
-                uint64(1),
-                address(delegateOwner),
-                false, // CALL
-                abi.encodeCall(DelegateOwner.setAdmin, (Emma))
-            )
-        );
-
-        vm.prank(Bob);
-        vm.expectRevert(DelegateOwner.DO_PERMISSION_DENIED.selector);
-        delegateOwner.invokeCall(data);
-
-        vm.prank(David);
-        delegateOwner.invokeCall(data);
-        assertEq(delegateOwner.nextTxId(), 2);
-        assertEq(delegateOwner.admin(), Emma);
-
-        vm.prank(Emma);
-        delegateOwner.setAdmin(Bob);
-        assertEq(delegateOwner.nextTxId(), 2);
-        assertEq(delegateOwner.admin(), Bob);
     }
 }

packages/protocol/test/bridge/Bridge.t.sol

@@ -40,7 +40,6 @@ contract BridgeTest is TaikoTest {
     SignalService signalService;
     SkipProofCheckSignal mockProofSignalService;
     UntrustedSendMessageRelayer untrustedSenderContract;
-    DelegateOwner delegateOwner;
 
     NonMaliciousContract1 nonmaliciousContract1;
     MaliciousContract2 maliciousContract2;
@@ -86,18 +85,6 @@ contract BridgeTest is TaikoTest {
         uint64 l1ChainId = uint64(block.chainid);
         vm.chainId(destChainId);
 
-        delegateOwner = DelegateOwner(
-            payable(
-                deployProxy({
-                    name: "delegate_owner",
-                    impl: address(new DelegateOwner()),
-                    data: abi.encodeCall(
-                        DelegateOwner.init, (mockDAO, address(addressManager), l1ChainId, address(0))
-                    )
-                })
-            )
-        );
-
         vm.chainId(l1ChainId);
 
         mockProofSignalService = SkipProofCheckSignal(
@@ -132,12 +119,6 @@ contract BridgeTest is TaikoTest {
 
         register(address(addressManager), "bridge_watchdog", address(uint160(123)), destChainId);
 
-        // Otherwise delegateOwner cannot do actions on them, on behalf of the DAO.
-        destChainBridge.transferOwnership(address(delegateOwner));
-        delegateOwner.acceptOwnership(address(destChainBridge));
-        mockProofSignalService.transferOwnership(address(delegateOwner));
-        delegateOwner.acceptOwnership(address(mockProofSignalService));
-
         vm.stopPrank();
     }