Change KeystoneForwarder routing gas accounting

.changeset/long-balloons-hunt.md

@@ -0,0 +1,5 @@
+---
+"chainlink": patch
+---
+
+#internal

contracts/.changeset/proud-pears-tie.md

@@ -0,0 +1,5 @@
+---
+'@chainlink/contracts': patch
+---
+
+#internal

contracts/src/v0.8/keystone/KeystoneForwarder.sol

@@ -108,9 +108,15 @@ contract KeystoneForwarder is OwnerIsCreator, ITypeAndVersion, IRouter {
   uint256 internal constant FORWARDER_METADATA_LENGTH = 45;
   uint256 internal constant SIGNATURE_LENGTH = 65;
 
-  /// @dev The gas we require to revert in case of a revert in the call to the
-  /// receiver. This is more than enough and does not attempt to be exact.
-  uint256 internal constant REQUIRED_GAS_FOR_ROUTING = 60_000;
+  /// @dev This is the gas required to store `success` after the report is processed.
+  /// It is a warm storage write because of the packed struct. In practice it will cost less.
+  uint256 internal constant INTERNAL_GAS_REQUIREMENTS_AFTER_REPORT = 5_000;
+  /// @dev This is the gas required to store the transmission struct and perform other checks.
+  uint256 internal constant INTERNAL_GAS_REQUIREMENTS = 25_000 + INTERNAL_GAS_REQUIREMENTS_AFTER_REPORT;
+  /// @dev This is the minimum gas required to route a report. This includes internal gas requirements
+  /// as well as the minimum gas that the user contract will receive. 30k * 3 gas is to account for
+  /// cases where consumers need close to the 30k limit provided in the supportsInterface check.
+  uint256 internal constant MINIMUM_GAS_LIMIT = INTERNAL_GAS_REQUIREMENTS + 30_000 * 3 + 10_000;
 
   // ================================================================
   // │                          Router                              │
@@ -137,16 +143,17 @@ contract KeystoneForwarder is OwnerIsCreator, ITypeAndVersion, IRouter {
     bytes calldata validatedReport
   ) public returns (bool) {
     if (!s_forwarders[msg.sender]) revert UnauthorizedForwarder();
-    uint256 gasLeft = gasleft();
-    if (gasLeft < REQUIRED_GAS_FOR_ROUTING) revert InsufficientGasForRouting(transmissionId);
+
+    uint256 gasLimit = gasleft() - INTERNAL_GAS_REQUIREMENTS;
+    if (gasLimit < MINIMUM_GAS_LIMIT) revert InsufficientGasForRouting(transmissionId);
 
     Transmission memory transmission = s_transmissions[transmissionId];
     if (transmission.success || transmission.invalidReceiver) revert AlreadyAttempted(transmissionId);
 
-    uint256 gasLimit = gasLeft - REQUIRED_GAS_FOR_ROUTING;
     s_transmissions[transmissionId].transmitter = transmitter;
     s_transmissions[transmissionId].gasLimit = uint80(gasLimit);
 
+    // This call can consume up to 90k gas.
     if (!ERC165Checker.supportsInterface(receiver, type(IReceiver).interfaceId)) {
       s_transmissions[transmissionId].invalidReceiver = true;
       return false;
@@ -155,10 +162,11 @@ contract KeystoneForwarder is OwnerIsCreator, ITypeAndVersion, IRouter {
     bool success;
     bytes memory payload = abi.encodeCall(IReceiver.onReport, (metadata, validatedReport));
 
+    uint256 remainingGas = gasleft() - INTERNAL_GAS_REQUIREMENTS;
     assembly {
       // call and return whether we succeeded. ignore return data
       // call(gas,addr,value,argsOffset,argsLength,retOffset,retLength)
-      success := call(gasLimit, receiver, 0, add(payload, 0x20), mload(payload), 0x0, 0x0)
+      success := call(remainingGas, receiver, 0, add(payload, 0x20), mload(payload), 0x0, 0x0)
     }
 
     if (success) {

contracts/src/v0.8/keystone/test/KeystoneForwarder_ReportTest.t.sol

@@ -3,7 +3,10 @@ pragma solidity 0.8.24;
 
 import {BaseTest} from "./KeystoneForwarderBaseTest.t.sol";
 import {IRouter} from "../interfaces/IRouter.sol";
+import {MaliciousReportReceiver} from "./mocks/MaliciousReportReceiver.sol";
+import {MaliciousRevertingReceiver} from "./mocks/MaliciousRevertingReceiver.sol";
 import {KeystoneForwarder} from "../KeystoneForwarder.sol";
+import {console} from "forge-std/console.sol";
 
 contract KeystoneForwarder_ReportTest is BaseTest {
   event MessageReceived(bytes metadata, bytes[] mercuryReports);
@@ -164,7 +167,7 @@ contract KeystoneForwarder_ReportTest is BaseTest {
   function test_RevertWhen_AttemptingTransmissionWithInsufficientGas() public {
     bytes32 transmissionId = s_forwarder.getTransmissionId(address(s_receiver), executionId, reportId);
     vm.expectRevert(abi.encodeWithSelector(IRouter.InsufficientGasForRouting.selector, transmissionId));
-    s_forwarder.report{gas: 50_000}(address(s_receiver), report, reportContext, signatures);
+    s_forwarder.report{gas: 150_000}(address(s_receiver), report, reportContext, signatures);
   }
 
   function test_Report_SuccessfulDelivery() public {
@@ -236,6 +239,38 @@ contract KeystoneForwarder_ReportTest is BaseTest {
     assertEq(uint8(transmissionInfo.state), uint8(IRouter.TransmissionState.INVALID_RECEIVER), "state mismatch");
   }
 
+  function test_Report_FailedDeliveryWhenReportReceiverConsumesAllGasAndInterfaceCheckUsesMax() public {
+    MaliciousRevertingReceiver maliciousReceiver = new MaliciousRevertingReceiver();
+    // This should not revert if gas tracking is effective
+    // It may revert if it fails to reserve sufficient gas for routing
+    // This POC requires pretty specific initial gas, so that 1/64 of gas passed to `onReport()` is insufficient to store the success
+    s_forwarder.report{gas: 200_000}(address(maliciousReceiver), report, reportContext, signatures);
+
+    IRouter.TransmissionInfo memory transmissionInfo = s_forwarder.getTransmissionInfo(
+      address(maliciousReceiver),
+      executionId,
+      reportId
+    );
+
+    assertEq(transmissionInfo.transmitter, TRANSMITTER, "transmitter mismatch");
+    assertEq(uint8(transmissionInfo.state), uint8(IRouter.TransmissionState.SUCCEEDED), "state mismatch");
+  }
+
+  function test_Report_FailedDelieryWhenReportReceiverConsumesAllGas() public {
+    MaliciousReportReceiver s_maliciousReceiver = new MaliciousReportReceiver();
+    s_forwarder.report{gas: 500_000}(address(s_maliciousReceiver), report, reportContext, signatures);
+
+    IRouter.TransmissionInfo memory transmissionInfo = s_forwarder.getTransmissionInfo(
+      address(s_maliciousReceiver),
+      executionId,
+      reportId
+    );
+
+    assertEq(transmissionInfo.transmitter, TRANSMITTER, "transmitter mismatch");
+    assertEq(uint8(transmissionInfo.state), uint8(IRouter.TransmissionState.FAILED), "state mismatch");
+    assertGt(transmissionInfo.gasLimit, 410_000, "gas limit mismatch");
+  }
+
   function test_Report_ConfigVersion() public {
     vm.stopPrank();
     // configure a new configVersion

contracts/src/v0.8/keystone/test/mocks/MaliciousReportReceiver.sol

@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.24;
+
+import {IERC165} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/interfaces/IERC165.sol";
+import {IReceiver} from "../../interfaces/IReceiver.sol";
+
+contract MaliciousReportReceiver is IReceiver, IERC165 {
+  event MessageReceived(bytes metadata, bytes[] mercuryReports);
+  bytes public latestReport;
+
+  function onReport(bytes calldata metadata, bytes calldata rawReport) external {
+    // Exhaust all gas that was provided
+    for (uint256 i = 0; i < 1_000_000_000; i++) {
+      bytes[] memory mercuryReports = abi.decode(rawReport, (bytes[]));
+      latestReport = rawReport;
+      emit MessageReceived(metadata, mercuryReports);
+    }
+  }
+
+  function supportsInterface(bytes4 interfaceId) public pure override returns (bool) {
+    return interfaceId == type(IReceiver).interfaceId || interfaceId == type(IERC165).interfaceId;
+  }
+}

contracts/src/v0.8/keystone/test/mocks/MaliciousRevertingReceiver.sol

@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.24;
+
+import {IERC165} from "../../../vendor/openzeppelin-solidity/v4.8.3/contracts/interfaces/IERC165.sol";
+import {IReceiver} from "../../interfaces/IReceiver.sol";
+import {Test} from "forge-std/Test.sol";
+
+/// A malicious receiver that uses max allowed for ERC165 checks and consumes all gas in `onReport()`
+/// Causes parent Forwarder contract to revert if it doesn't handle gas tracking accurately
+contract MaliciousRevertingReceiver is IReceiver, IERC165, Test {
+  function onReport(bytes calldata, bytes calldata) external view override {
+    // consumes about 63/64 of all gas available
+    uint256 targetGasRemaining = 200;
+    for (uint256 i = 0; gasleft() > targetGasRemaining; i++) {}
+  }
+
+  function supportsInterface(bytes4 interfaceId) public pure override returns (bool) {
+    // Consume up to the maximum amount of gas that can be consumed in this check
+    // This loop consumes roughly 29_000 gas
+    for (uint256 i = 0; i < 670; i++) {}
+
+    return interfaceId == type(IReceiver).interfaceId || interfaceId == type(IERC165).interfaceId;
+  }
+}

core/capabilities/targets/write_target.go

@@ -19,7 +19,7 @@ import (
 )
 
 var (
-	_ capabilities.ActionCapability = &WriteTarget{}
+	_ capabilities.TargetCapability = &WriteTarget{}
 )
 
 type WriteTarget struct {
@@ -48,7 +48,7 @@ type TransmissionInfo struct {
 // The gas cost of the forwarder contract logic, including state updates and event emission.
 // This is a rough estimate and should be updated if the forwarder contract logic changes.
 // TODO: Make this part of the on-chain capability configuration
-const FORWARDER_CONTRACT_LOGIC_GAS_COST = 100_000
+const ForwarderContractLogicGasCost = 100_000
 
 type ContractValueGetter interface {
 	Bind(context.Context, []commontypes.BoundContract) error
@@ -77,7 +77,7 @@ func NewWriteTarget(
 			Name:    "forwarder",
 		},
 		forwarderAddress,
-		txGasLimit - FORWARDER_CONTRACT_LOGIC_GAS_COST,
+		txGasLimit - ForwarderContractLogicGasCost,
 		info,
 		logger.Named(lggr, "WriteTarget"),
 		false,
@@ -252,7 +252,7 @@ func (cap *WriteTarget) Execute(ctx context.Context, rawRequest capabilities.Cap
 	case transmissionInfo.State == 3: // FAILED
 		receiverGasMinimum := cap.receiverGasMinimum
 		if request.Config.GasLimit != nil {
-			receiverGasMinimum = *request.Config.GasLimit - FORWARDER_CONTRACT_LOGIC_GAS_COST
+			receiverGasMinimum = *request.Config.GasLimit - ForwarderContractLogicGasCost
 		}
 		if transmissionInfo.GasLimit.Uint64() > receiverGasMinimum {
 			cap.lggr.Infow("returning without a transmission attempt - transmission already attempted and failed, sufficient gas was provided", "executionID", request.Metadata.WorkflowExecutionID, "receiverGasMinimum", receiverGasMinimum, "transmissionGasLimit", transmissionInfo.GasLimit)