[Functions] Minor contract fixes (#10511)

* (fix): Functions Subscriptions add consumer checks if sub consumers greater or equal to max to account for changes to the maximum

* (fix): Functions Router during fulfillment do not revert on invalid client

* Functions Coordinator oracleWithdrawAll checks for 0 balances

* (test): Add unit tests for changes

* Update gas snapshot

* Changes after rebase

contracts/gas-snapshots/functions.gas-snapshot

@@ -1,5 +1,7 @@
 FunctionsBilling_EstimateCost:test_EstimateCost_RevertsIfGasPriceAboveCeiling() (gas: 32391)
 FunctionsBilling_EstimateCost:test_EstimateCost_Success() (gas: 52979)
+FunctionsBilling_OracleWithdrawAll:test_OracleWithdrawAll_RevertIfNotOwner() (gas: 13274)
+FunctionsBilling_OracleWithdrawAll:test_OracleWithdrawAll_SuccessPaysTransmittersWithBalance() (gas: 147058)
 FunctionsOracle_sendRequest:testEmptyRequestDataReverts() (gas: 13452)
 FunctionsOracle_setDONPublicKey:testEmptyPublicKeyReverts() (gas: 10974)
 FunctionsOracle_setDONPublicKey:testOnlyOwnerReverts() (gas: 11255)
@@ -11,16 +13,17 @@ FunctionsOracle_setRegistry:testSetRegistrySuccess() (gas: 35791)
 FunctionsOracle_setRegistry:testSetRegistry_gas() (gas: 31987)
 FunctionsOracle_typeAndVersion:testTypeAndVersionSuccess() (gas: 6905)
 FunctionsRouter_Constructor:test_Constructor_Success() (gas: 12073)
-FunctionsRouter_Fulfill:test_Fulfill_RequestNotProcessedCostExceedsCommitment() (gas: 48299)
-FunctionsRouter_Fulfill:test_Fulfill_RequestNotProcessedInsufficientGas() (gas: 38830)
-FunctionsRouter_Fulfill:test_Fulfill_RequestNotProcessedInvalidCommitment() (gas: 36239)
+FunctionsRouter_Fulfill:test_Fulfill_RequestNotProcessedCostExceedsCommitment() (gas: 48277)
+FunctionsRouter_Fulfill:test_Fulfill_RequestNotProcessedInsufficientGas() (gas: 38808)
+FunctionsRouter_Fulfill:test_Fulfill_RequestNotProcessedInvalidCommitment() (gas: 36217)
 FunctionsRouter_Fulfill:test_Fulfill_RequestNotProcessedInvalidRequestId() (gas: 35161)
-FunctionsRouter_Fulfill:test_Fulfill_RequestNotProcessedSubscriptionBalanceInvariant() (gas: 165)
-FunctionsRouter_Fulfill:test_Fulfill_RevertIfNotCommittedCoordinator() (gas: 28037)
+FunctionsRouter_Fulfill:test_Fulfill_RequestNotProcessedSubscriptionBalanceInvariant() (gas: 210)
+FunctionsRouter_Fulfill:test_Fulfill_RevertIfNotCommittedCoordinator() (gas: 28015)
 FunctionsRouter_Fulfill:test_Fulfill_RevertIfPaused() (gas: 33206)
-FunctionsRouter_Fulfill:test_Fulfill_SuccessFulfilled() (gas: 103393)
-FunctionsRouter_Fulfill:test_Fulfill_SuccessUserCallbackReverts() (gas: 1762917)
-FunctionsRouter_Fulfill:test_Fulfill_SuccessUserCallbackRunsOutOfGas() (gas: 205990)
+FunctionsRouter_Fulfill:test_Fulfill_SuccessClientNoLongerExists() (gas: 93612)
+FunctionsRouter_Fulfill:test_Fulfill_SuccessFulfilled() (gas: 103408)
+FunctionsRouter_Fulfill:test_Fulfill_SuccessUserCallbackReverts() (gas: 1762929)
+FunctionsRouter_Fulfill:test_Fulfill_SuccessUserCallbackRunsOutOfGas() (gas: 206005)
 FunctionsRouter_GetAdminFee:test_GetAdminFee_Success() (gas: 17842)
 FunctionsRouter_GetAllowListId:test_GetAllowListId_Success() (gas: 12883)
 FunctionsRouter_GetConfig:test_GetConfig_Success() (gas: 31332)
@@ -44,7 +47,7 @@ FunctionsRouter_SendRequest:test_SendRequest_RevertIfConsumerNotAllowed() (gas:
 FunctionsRouter_SendRequest:test_SendRequest_RevertIfDuplicateRequestId() (gas: 192424)
 FunctionsRouter_SendRequest:test_SendRequest_RevertIfEmptyData() (gas: 29382)
 FunctionsRouter_SendRequest:test_SendRequest_RevertIfIncorrectDonId() (gas: 57929)
-FunctionsRouter_SendRequest:test_SendRequest_RevertIfInsufficientSubscriptionBalance() (gas: 186036)
+FunctionsRouter_SendRequest:test_SendRequest_RevertIfInsufficientSubscriptionBalance() (gas: 186033)
 FunctionsRouter_SendRequest:test_SendRequest_RevertIfInvalidCallbackGasLimit() (gas: 48353)
 FunctionsRouter_SendRequest:test_SendRequest_RevertIfInvalidDonId() (gas: 25038)
 FunctionsRouter_SendRequest:test_SendRequest_RevertIfNoSubscription() (gas: 29088)
@@ -58,7 +61,7 @@ FunctionsRouter_SendRequestToProposed:test_SendRequestToProposed_RevertIfInvalid
 FunctionsRouter_SendRequestToProposed:test_SendRequestToProposed_RevertIfNoSubscription() (gas: 35718)
 FunctionsRouter_SendRequestToProposed:test_SendRequestToProposed_RevertIfPaused() (gas: 40788)
 FunctionsRouter_SendRequestToProposed:test_SendRequestToProposed_Success() (gas: 204484)
-FunctionsRouter_SendRequestToProposed:test_SendRequest_RevertIfInsufficientSubscriptionBalance() (gas: 192600)
+FunctionsRouter_SendRequestToProposed:test_SendRequest_RevertIfInsufficientSubscriptionBalance() (gas: 192597)
 FunctionsRouter_SetAllowListId:test_SetAllowListId_Success() (gas: 30666)
 FunctionsRouter_SetAllowListId:test_UpdateConfig_RevertIfNotOwner() (gas: 13402)
 FunctionsRouter_Unpause:test_Unpause_RevertIfNotOwner() (gas: 13294)
@@ -72,12 +75,13 @@ FunctionsSubscriptions_AcceptSubscriptionOwnerTransfer:test_AcceptSubscriptionOw
 FunctionsSubscriptions_AcceptSubscriptionOwnerTransfer:test_AcceptSubscriptionOwnerTransfer_RevertIfSenderBecomesBlocked() (gas: 94703)
 FunctionsSubscriptions_AcceptSubscriptionOwnerTransfer:test_AcceptSubscriptionOwnerTransfer_RevertIfSenderIsNotNewOwner() (gas: 62713)
 FunctionsSubscriptions_AcceptSubscriptionOwnerTransfer:test_AcceptSubscriptionOwnerTransfer_Success() (gas: 59611)
-FunctionsSubscriptions_AddConsumer:test_AddConsumer_RevertIfMaximumConsumers() (gas: 137842)
-FunctionsSubscriptions_AddConsumer:test_AddConsumer_RevertIfNoSubscription() (gas: 12914)
+FunctionsSubscriptions_AddConsumer:test_AddConsumer_RevertIfMaximumConsumers() (gas: 137833)
+FunctionsSubscriptions_AddConsumer:test_AddConsumer_RevertIfMaximumConsumersAfterConfigUpdate() (gas: 161337)
+FunctionsSubscriptions_AddConsumer:test_AddConsumer_RevertIfNoSubscription() (gas: 12926)
 FunctionsSubscriptions_AddConsumer:test_AddConsumer_RevertIfNotAllowedSender() (gas: 57789)
 FunctionsSubscriptions_AddConsumer:test_AddConsumer_RevertIfNotSubscriptionOwner() (gas: 87166)
-FunctionsSubscriptions_AddConsumer:test_AddConsumer_RevertIfPaused() (gas: 18006)
-FunctionsSubscriptions_AddConsumer:test_AddConsumer_Success() (gas: 95394)
+FunctionsSubscriptions_AddConsumer:test_AddConsumer_RevertIfPaused() (gas: 18051)
+FunctionsSubscriptions_AddConsumer:test_AddConsumer_Success() (gas: 95391)
 FunctionsSubscriptions_CancelSubscription:test_CancelSubscription_RevertIfNoSubscription() (gas: 15041)
 FunctionsSubscriptions_CancelSubscription:test_CancelSubscription_RevertIfNotAllowedSender() (gas: 57863)
 FunctionsSubscriptions_CancelSubscription:test_CancelSubscription_RevertIfNotSubscriptionOwner() (gas: 89296)

contracts/src/v0.8/functions/dev/1_0_0/FunctionsBilling.sol

@@ -347,8 +347,10 @@ abstract contract FunctionsBilling is Routable, IFunctionsBilling {
     // Bounded by "maxNumOracles" on OCR2Abstract.sol
     for (uint256 i = 0; i < transmitters.length; ++i) {
       uint96 balance = s_withdrawableTokens[transmitters[i]];
-      s_withdrawableTokens[transmitters[i]] = 0;
-      IFunctionsSubscriptions(address(_getRouter())).oracleWithdraw(transmitters[i], balance);
+      if (balance > 0) {
+        s_withdrawableTokens[transmitters[i]] = 0;
+        IFunctionsSubscriptions(address(_getRouter())).oracleWithdraw(transmitters[i], balance);
+      }
     }
   }
 

contracts/src/v0.8/functions/dev/1_0_0/FunctionsRouter.sol

@@ -389,6 +389,18 @@ contract FunctionsRouter is IFunctionsRouter, FunctionsSubscriptions, Pausable,
     uint32 callbackGasLimit,
     address client
   ) private returns (CallbackResult memory) {
+    bool destinationNoLongerExists;
+    // solhint-disable-next-line no-inline-assembly
+    assembly {
+      // solidity calls check that a contract actually exists at the destination, so we do the same
+      destinationNoLongerExists := iszero(extcodesize(client))
+    }
+    if (destinationNoLongerExists) {
+      // Return without attempting callback
+      // The subscription will still be charged to reimburse transmitter's gas overhead
+      return CallbackResult({success: false, gasUsed: 0, returnData: new bytes(0)});
+    }
+
     bytes memory encodedCallback = abi.encodeWithSelector(
       s_config.handleOracleFulfillmentSelector,
       requestId,
@@ -410,13 +422,6 @@ contract FunctionsRouter is IFunctionsRouter, FunctionsSubscriptions, Pausable,
 
     // solhint-disable-next-line no-inline-assembly
     assembly {
-      // solidity calls check that a contract actually exists at the destination, so we do the same
-      // Note we do this check prior to measuring gas so gasForCallExactCheck (our "cushion")
-      // doesn't need to account for it.
-      if iszero(extcodesize(client)) {
-        revert(0, 0)
-      }
-
       let g := gas()
       // Compute g -= gasForCallExactCheck and check for underflow
       // The gas actually passed to the callee is _min(gasAmount, 63//64*gas available).

contracts/src/v0.8/functions/dev/1_0_0/FunctionsSubscriptions.sol

@@ -395,7 +395,7 @@ abstract contract FunctionsSubscriptions is IFunctionsSubscriptions, IERC677Rece
 
     // Already maxed, cannot add any more consumers.
     uint16 maximumConsumers = _getMaxConsumers();
-    if (s_subscriptions[subscriptionId].consumers.length == maximumConsumers) {
+    if (s_subscriptions[subscriptionId].consumers.length >= maximumConsumers) {
       revert TooManyConsumers(maximumConsumers);
     }
     if (s_consumers[consumer][subscriptionId].allowed) {

contracts/src/v0.8/functions/tests/1_0_0/FunctionsCoordinator.t.sol

@@ -5,7 +5,7 @@ import {FunctionsCoordinator} from "../../dev/1_0_0/FunctionsCoordinator.sol";
 import {FunctionsBilling} from "../../dev/1_0_0/FunctionsBilling.sol";
 import {FunctionsRequest} from "../../dev/1_0_0/libraries/FunctionsRequest.sol";
 
-import {FunctionsSubscriptionSetup} from "./Setup.t.sol";
+import {FunctionsSubscriptionSetup, FunctionsMultipleFulfillmentsSetup} from "./Setup.t.sol";
 
 // ================================================================
 // |                       Functions Coordinator                  |
@@ -209,8 +209,47 @@ contract FunctionsBilling_OracleWithdraw {
 }
 
 /// @notice #oracleWithdrawAll
-contract FunctionsBilling_OracleWithdrawAll {
+contract FunctionsBilling_OracleWithdrawAll is FunctionsMultipleFulfillmentsSetup {
+  function setUp() public virtual override {
+    // Use no DON fee so that a transmitter has a balance of 0
+    s_donFee = 0;
+
+    FunctionsMultipleFulfillmentsSetup.setUp();
+  }
 
+  function test_OracleWithdrawAll_RevertIfNotOwner() public {
+    // Send as stranger
+    vm.stopPrank();
+    vm.startPrank(STRANGER_ADDRESS);
+
+    vm.expectRevert("Only callable by owner");
+    s_functionsCoordinator.oracleWithdrawAll();
+  }
+
+  function test_OracleWithdrawAll_SuccessPaysTransmittersWithBalance() public {
+    uint256 transmitter1BalanceBefore = s_linkToken.balanceOf(NOP_TRANSMITTER_ADDRESS_1);
+    assertEq(transmitter1BalanceBefore, 0);
+    uint256 transmitter2BalanceBefore = s_linkToken.balanceOf(NOP_TRANSMITTER_ADDRESS_2);
+    assertEq(transmitter2BalanceBefore, 0);
+    uint256 transmitter3BalanceBefore = s_linkToken.balanceOf(NOP_TRANSMITTER_ADDRESS_3);
+    assertEq(transmitter3BalanceBefore, 0);
+    uint256 transmitter4BalanceBefore = s_linkToken.balanceOf(NOP_TRANSMITTER_ADDRESS_4);
+    assertEq(transmitter4BalanceBefore, 0);
+
+    s_functionsCoordinator.oracleWithdrawAll();
+
+    uint96 expectedTransmitterBalance = s_fulfillmentCoordinatorBalance / 3;
+
+    uint256 transmitter1BalanceAfter = s_linkToken.balanceOf(NOP_TRANSMITTER_ADDRESS_1);
+    assertEq(transmitter1BalanceAfter, expectedTransmitterBalance);
+    uint256 transmitter2BalanceAfter = s_linkToken.balanceOf(NOP_TRANSMITTER_ADDRESS_2);
+    assertEq(transmitter2BalanceAfter, expectedTransmitterBalance);
+    uint256 transmitter3BalanceAfter = s_linkToken.balanceOf(NOP_TRANSMITTER_ADDRESS_3);
+    assertEq(transmitter3BalanceAfter, expectedTransmitterBalance);
+    // Transmitter 4 has no balance
+    uint256 transmitter4BalanceAfter = s_linkToken.balanceOf(NOP_TRANSMITTER_ADDRESS_4);
+    assertEq(transmitter4BalanceAfter, 0);
+  }
 }
 
 /// @notice #_getTransmitters

contracts/src/v0.8/functions/tests/1_0_0/FunctionsRouter.t.sol

@@ -1191,6 +1191,53 @@ contract FunctionsRouter_Fulfill is FunctionsClientRequestSetup {
     assertEq(callbackGasCostJuels, 0);
   }
 
+  function test_Fulfill_SuccessClientNoLongerExists() public {
+    // Delete the Client contract in the time between request and fulfillment
+    vm.etch(address(s_functionsClient), new bytes(0));
+
+    // Send as committed Coordinator
+    vm.stopPrank();
+    vm.startPrank(address(s_functionsCoordinator));
+
+    bytes memory response = bytes("hello world!");
+    bytes memory err = new bytes(0);
+    uint96 juelsPerGas = 0;
+    uint96 costWithoutCallback = 0;
+    address transmitter = NOP_TRANSMITTER_ADDRESS_1;
+    FunctionsResponse.Commitment memory commitment = s_requestCommitment;
+
+    // topic0 (function signature, always checked), topic1 (true), topic2 (true), NOT topic3 (false), and data (true).
+    bool checkTopic1RequestId = true;
+    bool checkTopic2SubscriptionId = true;
+    bool checkTopic3 = false;
+    bool checkData = true;
+    vm.expectEmit(checkTopic1RequestId, checkTopic2SubscriptionId, checkTopic3, checkData);
+    emit RequestProcessed({
+      requestId: s_requestId,
+      subscriptionId: s_subscriptionId,
+      totalCostJuels: s_adminFee + costWithoutCallback, // NOTE: tx.gasprice is at 0, so no callback gas used
+      transmitter: transmitter,
+      resultCode: FunctionsResponse.FulfillResult.USER_CALLBACK_ERROR,
+      response: response,
+      err: err,
+      callbackReturnData: new bytes(0)
+    });
+
+    vm.recordLogs();
+
+    (FunctionsResponse.FulfillResult resultCode, uint96 callbackGasCostJuels) = s_functionsRouter.fulfill(
+      response,
+      err,
+      juelsPerGas,
+      costWithoutCallback,
+      transmitter,
+      commitment
+    );
+
+    assertEq(uint(resultCode), uint(FunctionsResponse.FulfillResult.USER_CALLBACK_ERROR));
+    assertEq(callbackGasCostJuels, 0);
+  }
+
   function test_Fulfill_SuccessFulfilled() public {
     // Send as committed Coordinator
     vm.stopPrank();

contracts/src/v0.8/functions/tests/1_0_0/FunctionsSubscriptions.t.sol

@@ -807,6 +807,24 @@ contract FunctionsSubscriptions_AddConsumer is FunctionsSubscriptionSetup {
     s_functionsRouter.addConsumer(s_subscriptionId, address(3));
   }
 
+  function test_AddConsumer_RevertIfMaximumConsumersAfterConfigUpdate() public {
+    // Fill Consumers to s_maxConsumersPerSubscription
+    // Already has one from setup
+    s_functionsRouter.addConsumer(s_subscriptionId, address(1));
+    s_functionsRouter.addConsumer(s_subscriptionId, address(2));
+
+    // Lower maxConsumersPerSubscription
+    s_maxConsumersPerSubscription = 1;
+    FunctionsRouter.Config memory newRouterConfig = getRouterConfig();
+    s_functionsRouter.updateConfig(newRouterConfig);
+
+    // .AddConsumer should still revert
+    vm.expectRevert(
+      abi.encodeWithSelector(FunctionsSubscriptions.TooManyConsumers.selector, s_maxConsumersPerSubscription)
+    );
+    s_functionsRouter.addConsumer(s_subscriptionId, address(3));
+  }
+
   event SubscriptionConsumerAdded(uint64 indexed subscriptionId, address consumer);
 
   function test_AddConsumer_Success() public {

contracts/src/v0.8/functions/tests/1_0_0/Setup.t.sol

@@ -264,3 +264,134 @@ contract FunctionsFulfillmentSetup is FunctionsClientRequestSetup {
     vm.startPrank(OWNER_ADDRESS);
   }
 }
+
+contract FunctionsMultipleFulfillmentsSetup is FunctionsFulfillmentSetup {
+  bytes32 s_requestId2;
+  FunctionsResponse.Commitment s_requestCommitment2;
+  bytes32 s_requestId3;
+  FunctionsResponse.Commitment s_requestCommitment3;
+
+  function setUp() public virtual override {
+    FunctionsFulfillmentSetup.setUp();
+
+    // Make 2 additional requests (1 already complete)
+
+    //  *** Request #2 ***
+    vm.recordLogs();
+    s_requestId2 = s_functionsClient.sendRequest(
+      s_donId,
+      "return 'hello world';",
+      new bytes(0),
+      new string[](0),
+      new bytes[](0),
+      s_subscriptionId,
+      5500
+    );
+
+    // Get commitment data from OracleRequest event log
+    Vm.Log[] memory entriesAfterRequest2 = vm.getRecordedLogs();
+    (, , , , , , , FunctionsResponse.Commitment memory commitment2) = abi.decode(
+      entriesAfterRequest2[0].data,
+      (address, uint64, address, bytes, uint16, bytes32, uint64, FunctionsResponse.Commitment)
+    );
+    s_requestCommitment2 = commitment2;
+
+    // Transmit as transmitter 2
+    vm.stopPrank();
+    vm.startPrank(NOP_TRANSMITTER_ADDRESS_2);
+
+    // Build report
+    bytes32[] memory requestIds2 = new bytes32[](1);
+    requestIds2[0] = s_requestId2;
+    bytes[] memory results2 = new bytes[](1);
+    results2[0] = bytes("hello world!");
+    bytes[] memory errors2 = new bytes[](1);
+    // No error
+    bytes[] memory onchainMetadata2 = new bytes[](1);
+    onchainMetadata2[0] = abi.encode(s_requestCommitment2);
+    bytes[] memory offchainMetadata2 = new bytes[](1);
+    // No offchain metadata
+    bytes memory report2 = abi.encode(requestIds2, results2, errors2, onchainMetadata2, offchainMetadata2);
+
+    // Build signers
+    address[31] memory signers2;
+    signers2[0] = NOP_SIGNER_ADDRESS_2;
+
+    // Send report
+    vm.recordLogs();
+    s_functionsCoordinator.callReportWithSigners(report2, signers2);
+
+    // Get actual cost from RequestProcessed event log
+    Vm.Log[] memory entriesAfterFulfill2 = vm.getRecordedLogs();
+    (uint96 totalCostJuels2, , , , , ) = abi.decode(
+      entriesAfterFulfill2[2].data,
+      (uint96, address, FunctionsResponse.FulfillResult, bytes, bytes, bytes)
+    );
+    // totalCostJuels = costWithoutCallbackJuels + adminFee + callbackGasCostJuels
+    s_fulfillmentCoordinatorBalance += totalCostJuels2 - s_adminFee;
+    s_fulfillmentRouterOwnerBalance += s_adminFee;
+
+    // Return prank to Owner
+    vm.stopPrank();
+    vm.startPrank(OWNER_ADDRESS);
+
+    //  *** Request #3 ***
+    vm.recordLogs();
+    s_requestId3 = s_functionsClient.sendRequest(
+      s_donId,
+      "return 'hello world';",
+      new bytes(0),
+      new string[](0),
+      new bytes[](0),
+      s_subscriptionId,
+      5500
+    );
+
+    // Get commitment data from OracleRequest event log
+    Vm.Log[] memory entriesAfterRequest3 = vm.getRecordedLogs();
+    (, , , , , , , FunctionsResponse.Commitment memory commitment3) = abi.decode(
+      entriesAfterRequest3[0].data,
+      (address, uint64, address, bytes, uint16, bytes32, uint64, FunctionsResponse.Commitment)
+    );
+    s_requestCommitment3 = commitment3;
+
+    // Transmit as transmitter 3
+    vm.stopPrank();
+    vm.startPrank(NOP_TRANSMITTER_ADDRESS_3);
+
+    // Build report
+    bytes32[] memory requestIds3 = new bytes32[](1);
+    requestIds3[0] = s_requestId3;
+    bytes[] memory results3 = new bytes[](1);
+    results3[0] = bytes("hello world!");
+    bytes[] memory errors3 = new bytes[](1);
+    // No error
+    bytes[] memory onchainMetadata3 = new bytes[](1);
+    onchainMetadata3[0] = abi.encode(s_requestCommitment3);
+    bytes[] memory offchainMetadata3 = new bytes[](1);
+    // No offchain metadata
+    bytes memory report3 = abi.encode(requestIds3, results3, errors3, onchainMetadata3, offchainMetadata3);
+
+    // Build signers
+    address[31] memory signers3;
+    signers3[0] = NOP_SIGNER_ADDRESS_3;
+
+    // Send report
+    vm.recordLogs();
+    s_functionsCoordinator.callReportWithSigners(report3, signers3);
+
+    // Get actual cost from RequestProcessed event log
+    Vm.Log[] memory entriesAfterFulfill3 = vm.getRecordedLogs();
+    (uint96 totalCostJuels3, , , , , ) = abi.decode(
+      entriesAfterFulfill3[2].data,
+      (uint96, address, FunctionsResponse.FulfillResult, bytes, bytes, bytes)
+    );
+
+    // totalCostJuels = costWithoutCallbackJuels + adminFee + callbackGasCostJuels
+    s_fulfillmentCoordinatorBalance += totalCostJuels3 - s_adminFee;
+
+    // Return prank to Owner
+    vm.stopPrank();
+    vm.startPrank(OWNER_ADDRESS);
+  }
+}