fix(protocol): fix Bridge message.fee double spending bug (#17446)

taikoxyz · May 31, 2024 · 1bd3285 · 1bd3285
1 parent 538f2a6
commit 1bd3285
Show file tree

Hide file tree

Showing 4 changed files with 43 additions and 65 deletions.
diff --git a/packages/protocol/contracts/bridge/Bridge.sol b/packages/protocol/contracts/bridge/Bridge.sol
@@ -242,54 +242,50 @@ contract Bridge is EssentialContract, IBridge {
         stats.numCacheOps =
             _proveSignalReceived(signalService, msgHash, _message.srcChainId, _proof);
 
-        if (!_consumeEtherQuota(_message.value + _message.fee)) {
-            if (msg.sender != _message.destOwner) revert B_OUT_OF_ETH_QUOTA();
-            status_ = Status.RETRIABLE;
-            reason_ = StatusReason.OUT_OF_ETH_QUOTA;
+        if (!_consumeEtherQuota(_message.value + _message.fee)) revert B_OUT_OF_ETH_QUOTA();
+
+        uint256 refundAmount;
+        if (_unableToInvokeMessageCall(_message, signalService)) {
+            // Handle special addresses that don't require actual invocation but
+            // mark message as DONE
+            refundAmount = _message.value;
+            status_ = Status.DONE;
+            reason_ = StatusReason.INVOCATION_PROHIBITED;
         } else {
-            uint256 refundAmount;
-            if (_unableToInvokeMessageCall(_message, signalService)) {
-                // Handle special addresses that don't require actual invocation but
-                // mark message as DONE
-                refundAmount = _message.value;
+            uint256 gasLimit = msg.sender == _message.destOwner
+                ? gasleft() // ignore _message.gasLimit
+                : _invocationGasLimit(_message, true);
+
+            if (_invokeMessageCall(_message, msgHash, gasLimit)) {
                 status_ = Status.DONE;
-                reason_ = StatusReason.INVOCATION_PROHIBITED;
+                reason_ = StatusReason.INVOCATION_OK;
             } else {
-                uint256 gasLimit = msg.sender == _message.destOwner
-                    ? gasleft() // ignore _message.gasLimit
-                    : _invocationGasLimit(_message, true);
-
-                if (_invokeMessageCall(_message, msgHash, gasLimit)) {
-                    status_ = Status.DONE;
-                    reason_ = StatusReason.INVOCATION_OK;
-                } else {
-                    status_ = Status.RETRIABLE;
-                    reason_ = StatusReason.INVOCATION_FAILED;
-                }
+                status_ = Status.RETRIABLE;
+                reason_ = StatusReason.INVOCATION_FAILED;
             }
+        }
 
-            if (_message.fee != 0) {
-                refundAmount += _message.fee;
-
-                if (msg.sender != _message.destOwner && _message.gasLimit != 0) {
-                    unchecked {
-                        uint256 refund = stats.numCacheOps * _GAS_REFUND_PER_CACHE_OPERATION;
-                        stats.gasUsedInFeeCalc = uint32(GAS_OVERHEAD + gasStart - gasleft());
-                        uint256 gasCharged = refund.max(stats.gasUsedInFeeCalc) - refund;
-                        uint256 maxFee = gasCharged * _message.fee / _message.gasLimit;
-                        uint256 baseFee = gasCharged * block.basefee;
-                        uint256 fee =
-                            (baseFee >= maxFee ? maxFee : (maxFee + baseFee) >> 1).min(_message.fee);
-
-                        refundAmount -= fee;
-                        msg.sender.sendEtherAndVerify(fee, _SEND_ETHER_GAS_LIMIT);
-                    }
+        if (_message.fee != 0) {
+            refundAmount += _message.fee;
+
+            if (msg.sender != _message.destOwner && _message.gasLimit != 0) {
+                unchecked {
+                    uint256 refund = stats.numCacheOps * _GAS_REFUND_PER_CACHE_OPERATION;
+                    stats.gasUsedInFeeCalc = uint32(GAS_OVERHEAD + gasStart - gasleft());
+                    uint256 gasCharged = refund.max(stats.gasUsedInFeeCalc) - refund;
+                    uint256 maxFee = gasCharged * _message.fee / _message.gasLimit;
+                    uint256 baseFee = gasCharged * block.basefee;
+                    uint256 fee =
+                        (baseFee >= maxFee ? maxFee : (maxFee + baseFee) >> 1).min(_message.fee);
+
+                    refundAmount -= fee;
+                    msg.sender.sendEtherAndVerify(fee, _SEND_ETHER_GAS_LIMIT);
                 }
             }
-
-            _message.destOwner.sendEtherAndVerify(refundAmount, _SEND_ETHER_GAS_LIMIT);
         }
 
+        _message.destOwner.sendEtherAndVerify(refundAmount, _SEND_ETHER_GAS_LIMIT);
+
         _updateMessageStatus(msgHash, status_);
         emit MessageProcessed(msgHash, _message, stats);
     }
@@ -308,12 +304,11 @@ contract Bridge is EssentialContract, IBridge {
         bytes32 msgHash = hashMessage(_message);
         _checkStatus(msgHash, Status.RETRIABLE);
 
-        uint256 amount = _message.value + _message.fee;
-        if (!_consumeEtherQuota(amount)) revert B_OUT_OF_ETH_QUOTA();
+        if (!_consumeEtherQuota(_message.value)) revert B_OUT_OF_ETH_QUOTA();
 
         bool succeeded;
         if (_unableToInvokeMessageCall(_message, resolve(LibStrings.B_SIGNAL_SERVICE, false))) {
-            succeeded = _message.destOwner.sendEther(amount, _SEND_ETHER_GAS_LIMIT, "");
+            succeeded = _message.destOwner.sendEther(_message.value, _SEND_ETHER_GAS_LIMIT, "");
         } else {
             uint256 invocationGasLimit;
             if (msg.sender != _message.destOwner) {

diff --git a/packages/protocol/deployments/mainnet-contract-logs-L1.md b/packages/protocol/deployments/mainnet-contract-logs-L1.md
@@ -63,7 +63,7 @@
 
 - ens: `bridge.based.taiko.eth`
 - proxy: `0xd60247c6848B7Ca29eDdF63AA924E53dB6Ddd8EC`
-- impl: `0x71c2f41AEDe913AAEf2c62596E03702E348D6Cd0`
+- impl: `0x951B7Ae1bB26d12dB37f01748e8fB62FEf45A8B5`
 - owner: `admin.taiko.eth`
 - logs:
   - deployed on May 1, 2024 @commit`56dddf2b6`
@@ -74,6 +74,7 @@
   - upgraded from `0xc71CC3B0a47149878fad337fb2ca54E546A645ba` to `0x02F21B4C3d4dbfF70cE851741175a727c8D782Be` @commit`fa481c1` in @tx`0x02ed558762eae5f0a930ba4a1047a02d4a793ea48890268c32df04e882f138ff`
   - unpaused on 27 May, 2024 @tx`0x71ce1e61f1e42e34c9a51f5671ac260f2ac398e016ae645f2661f074e7f230ce`
   - upgraded from `0x02F21B4C3d4dbfF70cE851741175a727c8D782Be` to `0x71c2f41AEDe913AAEf2c62596E03702E348D6Cd0.` @commit`` in @tx`0x8a380a25d03a740d9535dfc3e2fc4f6960e22d49ad88b8d85f59af4013aedf87`
+  - upgrade impl to `0x951B7Ae1bB26d12dB37f01748e8fB62FEf45A8B5` @tx`0xf21f6bf720767db3bc9b63ef69cacb20340bdedfb6589e6a4d11fe082dfa7bd6`
 
 #### quota_manager
 

diff --git a/packages/protocol/deployments/mainnet-contract-logs-L2.md b/packages/protocol/deployments/mainnet-contract-logs-L2.md
@@ -41,7 +41,7 @@
 #### bridge
 
 - proxy: `0x1670000000000000000000000000000000000001`
-- impl: `0x98C5De7670aA7d47C6c0551fAD27Bfe464A6751a.`
+- impl: `0x0893c8821Fa358D5f3630695Ce062204814359A1.`
 - owner: `0xf8ff2AF0DC1D5BA4811f22aCb02936A1529fd2Be`
 - todo:
   - change owner to DelegateOwner
@@ -50,6 +50,7 @@
   - redeployed on May 22, 2024 @commit `b955e0e`
   - upgrade to `0xf961854D68368cFFc86d90AEe8a19E9781dACA3e` @tx`0x094dd9452d79cbd74711f2b8065566e4431a05d0727c56d2b38195e40fd62805`
   - upgraded from `0xf961854D68368cFFc86d90AEe8a19E9781dACA3e` to `0x98C5De7670aA7d47C6c0551fAD27Bfe464A6751a..` @commit`` in @tx`0x0b5d6acc9c5b8ef193920246081ec5ce7268111acfc1dce1f058bea06f3953c7`
+  - upgrade impl to `0x0893c8821Fa358D5f3630695Ce062204814359A1` @tx`0x4605c4ce594e996bdbdb532a9aefe4fab1ea36f7e2ef63eef56a7e8033810df3`
 
 #### erc20_vault
 

diff --git a/packages/protocol/test/bridge/Bridge2_processMessage.t.sol b/packages/protocol/test/bridge/Bridge2_processMessage.t.sol
@@ -422,11 +422,8 @@ contract BridgeTest2_processMessage is BridgeTest2 {
         bridge.processMessage(message, fakeProof);
 
         vm.prank(Alice);
+        vm.expectRevert(Bridge.B_OUT_OF_ETH_QUOTA.selector);
         bridge.processMessage(message, fakeProof);
-        bytes32 hash = bridge.hashMessage(message);
-        assertTrue(bridge.messageStatus(hash) == IBridge.Status.RETRIABLE);
-
-        assertEq(davidBalance, David.balance);
     }
 
     function test_bridge2_processMessage_and_retryMessage_malicious_way()
@@ -457,23 +454,7 @@ contract BridgeTest2_processMessage is BridgeTest2 {
         uint256 davidBalance = David.balance;
 
         vm.prank(Alice);
+        vm.expectRevert(Bridge.B_OUT_OF_ETH_QUOTA.selector);
         bridge.processMessage(message, fakeProof);
-        bytes32 hash = bridge.hashMessage(message);
-        assertTrue(bridge.messageStatus(hash) == IBridge.Status.RETRIABLE);
-
-        // Allow now quota
-        vm.startPrank(owner);
-        addressManager.setAddress(
-            uint64(block.chainid), "quota_manager", address(new AlwaysAvailableQuotaManager())
-        );
-        vm.stopPrank();
-
-        uint256 aliceBalanceBeforeRefund = Alice.balance;
-        vm.prank(message.destOwner);
-        bridge.retryMessage(message, false);
-
-        assertTrue(bridge.messageStatus(hash) == IBridge.Status.DONE);
-        // She could get back the ether but cannot execute the malicious call.
-        assertEq(Alice.balance, aliceBalanceBeforeRefund + message.value + message.fee);
     }
 }