Fixing post merge fixes (smartcontractkit#268)

Co-authored-by: AnieeG <[email protected]>
Co-authored-by: Rens Rooimans <[email protected]>
Co-authored-by: dimitris <[email protected]>
Co-authored-by: Jean Arnaud <[email protected]>
Co-authored-by: Chunkai Yang <[email protected]>
Co-authored-by: Connor Stein <[email protected]>

.github/workflows/solidity-foundry.yml

@@ -28,31 +28,6 @@ jobs:
               - '.gitmodules'
               - 'contracts/foundry-lib'
 
-  solhint:
-    needs: [changes]
-    name: Solhint
-    runs-on: ubuntu-latest
-
-    steps:
-      - name: Collect Metrics
-        if: ${{ needs.changes.outputs.changes == 'true' }}
-        id: collect-gha-metrics
-        uses: smartcontractkit/push-gha-metrics-action@8163dcea2f01a0a8fec84b284406ff7af1d2e1c0
-        with:
-          basic-auth: ${{ secrets.GRAFANA_CLOUD_BASIC_AUTH }}
-          hostname: ${{ secrets.GRAFANA_CLOUD_HOST }}
-          this-job-name: Solhint
-        continue-on-error: true
-      - name: Checkout the repo
-        uses: actions/checkout@24cb9080177205b6e8c946b17badbe402adc938f # v3.4.0
-      - name: Setup NodeJS
-        if: ${{ needs.changes.outputs.changes == 'true' }}
-        uses: ./.github/actions/setup-nodejs
-      - name: Run Solhint
-        if: ${{ needs.changes.outputs.changes == 'true' }}
-        working-directory: contracts
-        run: pnpm lint:ccip
-
   coverage:
     needs: [changes]
     name: Coverage
@@ -111,7 +86,7 @@ jobs:
         with:
           update-comment: true
           coverage-files: lcov.info.pruned
-          minimum-coverage: 98
+          minimum-coverage: 98.5
           artifact-name: code-coverage-report
           working-directory: ./contracts
           github-token: ${{ secrets.GITHUB_TOKEN }}

.gitmodules

@@ -1,3 +1,3 @@
 [submodule "contracts/foundry-lib/forge-std"]
 	path = contracts/foundry-lib/forge-std
-	url = https://github.com/foundry-rs/forge-std
+	url = https://github.com/foundry-rs/forge-std

contracts/.solhintignore

@@ -1,4 +1,4 @@
-# 351 warnings
+# 377 warnings
 #./src/v0.8/automation
 # 27 warnings
 #./src/v0.8/functions
@@ -11,6 +11,7 @@
 ./src/v0.8/llo-feeds/test
 ./src/v0.8/vrf/testhelpers
 ./src/v0.8/functions/tests
+./src/v0.8/ccip/test
 
 # Always ignore vendor
 ./src/v0.8/vendor

contracts/GNUmakefile

@@ -39,7 +39,7 @@ abigen: ## Build & install abigen.
 
 .PHONY: mockery
 mockery: $(mockery) ## Install mockery.
-	go install github.com/vektra/mockery/v2@v2.28.1
+	go install github.com/vektra/mockery/v2@v2.35.4
 
 .PHONY: foundry
 foundry: ## Install foundry.
@@ -56,7 +56,7 @@ ccip-precommit:
 	forge test
 	make snapshot
 	pnpm prettier:write
-	pnpm lint:ccip
+	pnpm solhint
 
 ccip-lcov: export FOUNDRY_PROFILE=ccip
 .PHONY: ccip-lcov

contracts/gas-snapshots/functions.gas-snapshot

@@ -8,16 +8,6 @@ FunctionsClient_FulfillRequest:test_FulfillRequest_MaximumGas() (gas: 498113)
 FunctionsClient_FulfillRequest:test_FulfillRequest_MinimumGas() (gas: 199261)
 FunctionsClient__SendRequest:test__SendRequest_RevertIfInvalidCallbackGasLimit() (gas: 54991)
 FunctionsCoordinator_Constructor:test_Constructor_Success() (gas: 11933)
-FunctionsOracle_sendRequest:testEmptyRequestDataReverts() (gas: 13452)
-FunctionsOracle_setDONPublicKey:testEmptyPublicKeyReverts() (gas: 10974)
-FunctionsOracle_setDONPublicKey:testOnlyOwnerReverts() (gas: 11255)
-FunctionsOracle_setDONPublicKey:testSetDONPublicKeySuccess() (gas: 126453)
-FunctionsOracle_setDONPublicKey:testSetDONPublicKey_gas() (gas: 97580)
-FunctionsOracle_setRegistry:testEmptyPublicKeyReverts() (gas: 10635)
-FunctionsOracle_setRegistry:testOnlyOwnerReverts() (gas: 10927)
-FunctionsOracle_setRegistry:testSetRegistrySuccess() (gas: 35791)
-FunctionsOracle_setRegistry:testSetRegistry_gas() (gas: 31987)
-FunctionsOracle_typeAndVersion:testTypeAndVersionSuccess() (gas: 6905)
 FunctionsRequest_DEFAULT_BUFFER_SIZE:test_DEFAULT_BUFFER_SIZE() (gas: 246)
 FunctionsRequest_EncodeCBOR:test_EncodeCBOR_Success() (gas: 223)
 FunctionsRequest_REQUEST_DATA_VERSION:test_REQUEST_DATA_VERSION() (gas: 225)
@@ -128,6 +118,7 @@ FunctionsSubscriptions_OwnerCancelSubscription:test_OwnerCancelSubscription_Reve
 FunctionsSubscriptions_OwnerCancelSubscription:test_OwnerCancelSubscription_Success() (gas: 54867)
 FunctionsSubscriptions_OwnerCancelSubscription:test_OwnerCancelSubscription_SuccessDeletesSubscription() (gas: 49624)
 FunctionsSubscriptions_OwnerCancelSubscription:test_OwnerCancelSubscription_SuccessSubOwnerRefunded() (gas: 50896)
+FunctionsSubscriptions_OwnerCancelSubscription:test_OwnerCancelSubscription_SuccessWhenRequestInFlight() (gas: 164300)
 FunctionsSubscriptions_OwnerWithdraw:test_OwnerWithdraw_RevertIfAmountMoreThanBalance() (gas: 17924)
 FunctionsSubscriptions_OwnerWithdraw:test_OwnerWithdraw_RevertIfBalanceInvariant() (gas: 210)
 FunctionsSubscriptions_OwnerWithdraw:test_OwnerWithdraw_RevertIfNotOwner() (gas: 15533)

contracts/gas-snapshots/shared.gas-snapshot

@@ -25,6 +25,10 @@ BurnMintERC677_mint:testSenderNotMinterReverts() (gas: 11195)
 BurnMintERC677_supportsInterface:testConstructorSuccess() (gas: 8685)
 BurnMintERC677_transfer:testInvalidAddressReverts() (gas: 10639)
 BurnMintERC677_transfer:testTransferSuccess() (gas: 39462)
+CallWithExactGas__callWithExactGasSafeReturnData:test_CallWithExactGasSafeReturnDataExactGas() (gas: 18955)
+CallWithExactGas__callWithExactGasSafeReturnData:test_NoContractReverts() (gas: 13887)
+CallWithExactGas__callWithExactGasSafeReturnData:test_NoGasForCallExactCheckReverts() (gas: 13177)
+CallWithExactGas__callWithExactGasSafeReturnData:test_NotEnoughGasForCallReverts() (gas: 13608)
 EnumerableMapAddresses_at:testAtSuccess() (gas: 98813)
 EnumerableMapAddresses_contains:testContainsSuccess() (gas: 97480)
 EnumerableMapAddresses_get:testGetSuccess() (gas: 98781)

contracts/package.json

@@ -18,7 +18,8 @@
     "prepublishOnly": "pnpm compile && ./scripts/prepublish_generate_abi_folder",
     "publish-beta": "pnpm publish --tag beta",
     "publish-prod": "npm dist-tag add @chainlink/[email protected] latest",
-    "lint:ccip": "solhint --config ./src/v0.8/ccip/.solhint.json --ignore-path .ccip-solhint-ignore --max-warnings 0 \"./src/v0.8/ccip/**/*.sol\""
+    "lint:ccip": "solhint --config ./src/v0.8/ccip/.solhint.json --ignore-path .ccip-solhint-ignore --max-warnings 0 \"./src/v0.8/ccip/**/*.sol\"",
+    "solhint": "solhint --max-warnings 404 \"./src/v0.8/**/*.sol\""
   },
   "files": [
     "src/v0.8/ccip/**/*.sol",

contracts/scripts/native_solc_compile_all_ccip

@@ -40,6 +40,8 @@ compileContract ccip/Router.sol
 compileContract ccip/PriceRegistry.sol
 compileContract ccip/pools/LockReleaseTokenPool.sol
 compileContract ccip/pools/BurnMintTokenPool.sol
+compileContract ccip/pools/BurnFromMintTokenPool.sol
+compileContract ccip/pools/BurnWithFromMintTokenPool.sol
 compileContract shared/token/ERC677/BurnMintERC677.sol
 compileContract ccip/ARM.sol
 compileContract ccip/ARMProxy.sol

contracts/src/v0.8/ccip/.solhint.json

@@ -1,43 +0,0 @@
-{
-  "extends": "solhint:recommended",
-  "plugins": ["prettier", "chainlink-solidity"],
-  "rules": {
-    "compiler-version": ["off", "^0.8.0"],
-    "const-name-snakecase": "off",
-    "constructor-syntax": "error",
-    "var-name-mixedcase": "off",
-    "func-named-parameters": "off",
-    "immutable-vars-naming": "off",
-    "func-visibility": [
-      "error",
-      {
-        "ignoreConstructors": true
-      }
-    ],
-    "not-rely-on-time": "off",
-    "prettier/prettier": [
-      "off",
-      {
-        "endOfLine": "auto"
-      }
-    ],
-    "no-empty-blocks": "off",
-    "quotes": ["error", "double"],
-    "reason-string": [
-      "warn",
-      {
-        "maxLength": 64
-      }
-    ],
-    "chainlink-solidity/prefix-internal-functions-with-underscore": "warn",
-    "chainlink-solidity/prefix-private-functions-with-underscore": "warn",
-    "chainlink-solidity/prefix-storage-variables-with-s-underscore": "warn",
-    "chainlink-solidity/prefix-immutable-variables-with-i": "warn",
-    "chainlink-solidity/all-caps-constant-storage-variables": "warn",
-    "chainlink-solidity/no-hardhat-imports": "warn",
-    "chainlink-solidity/inherited-constructor-args-not-in-contract-definition": "warn",
-    "chainlink-solidity/explicit-imports": "warn",
-    "chainlink-solidity/no-require-statements": "warn",
-    "chainlink-solidity/no-block-single-if-reverts": "warn"
-  }
-}

contracts/src/v0.8/ccip/Router.sol

@@ -11,8 +11,8 @@ import {IAny2EVMMessageReceiver} from "./interfaces/IAny2EVMMessageReceiver.sol"
 
 import {Client} from "./libraries/Client.sol";
 import {Internal} from "./libraries/Internal.sol";
-import {CallWithExactGas} from "./libraries/CallWithExactGas.sol";
-import {OwnerIsCreator} from "./../shared/access/OwnerIsCreator.sol";
+import {CallWithExactGas} from "../shared/call/CallWithExactGas.sol";
+import {OwnerIsCreator} from "../shared/access/OwnerIsCreator.sol";
 
 import {EnumerableSet} from "../vendor/openzeppelin-solidity/v4.8.0/contracts/utils/structs/EnumerableSet.sol";
 import {SafeERC20} from "../vendor/openzeppelin-solidity/v4.8.0/contracts/token/ERC20/utils/SafeERC20.sol";
@@ -156,24 +156,24 @@ contract Router is IRouter, IRouterClient, ITypeAndVersion, OwnerIsCreator {
     uint16 gasForCallExactCheck,
     uint256 gasLimit,
     address receiver
-  ) external override whenHealthy returns (bool success, bytes memory retData) {
+  ) external override whenHealthy returns (bool success, bytes memory retData, uint256 gasUsed) {
     // We only permit offRamps to call this function.
     if (!isOffRamp(message.sourceChainSelector, msg.sender)) revert OnlyOffRamp();
 
     // We encode here instead of the offRamps to constrain specifically what functions
     // can be called from the router.
     bytes memory data = abi.encodeWithSelector(IAny2EVMMessageReceiver.ccipReceive.selector, message);
 
-    (success, retData) = CallWithExactGas._callWithExactGas(
+    (success, retData, gasUsed) = CallWithExactGas._callWithExactGasSafeReturnData(
       data,
       receiver,
       gasLimit,
-      Internal.MAX_RET_BYTES,
-      gasForCallExactCheck
+      gasForCallExactCheck,
+      Internal.MAX_RET_BYTES
     );
 
     emit MessageExecuted(message.messageId, message.sourceChainSelector, msg.sender, keccak256(data));
-    return (success, retData);
+    return (success, retData, gasUsed);
   }
 
   // @notice Merges a chain selector and offRamp address into a single uint256 by shifting the

contracts/src/v0.8/ccip/interfaces/IRouter.sol

@@ -15,10 +15,11 @@ interface IRouter {
   /// the contract is called. If not, only tokens are transferred.
   /// @return success A boolean value indicating whether the ccip message was received without errors.
   /// @return retBytes A bytes array containing return data form CCIP receiver.
+  /// @return gasUsed the gas used by the external customer call. Does not include any overhead.
   function routeMessage(
     Client.Any2EVMMessage calldata message,
     uint16 gasForCallExactCheck,
     uint256 gasLimit,
     address receiver
-  ) external returns (bool success, bytes memory retBytes);
+  ) external returns (bool success, bytes memory retBytes, uint256 gasUsed);
 }

contracts/src/v0.8/ccip/offRamp/EVM2EVMOffRamp.sol

@@ -13,7 +13,7 @@ import {IAny2EVMOffRamp} from "../interfaces/IAny2EVMOffRamp.sol";
 import {Client} from "../libraries/Client.sol";
 import {Internal} from "../libraries/Internal.sol";
 import {RateLimiter} from "../libraries/RateLimiter.sol";
-import {CallWithExactGas} from "../libraries/CallWithExactGas.sol";
+import {CallWithExactGas} from "../../shared/call/CallWithExactGas.sol";
 import {OCR2BaseNoChecks} from "../ocr/OCR2BaseNoChecks.sol";
 import {AggregateRateLimiter} from "../AggregateRateLimiter.sol";
 import {EnumerableMapAddresses} from "../../shared/enumerable/EnumerableMapAddresses.sol";
@@ -430,7 +430,7 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
       !message.receiver.isContract() || !message.receiver.supportsInterface(type(IAny2EVMMessageReceiver).interfaceId)
     ) return;
 
-    (bool success, bytes memory returnData) = IRouter(s_dynamicConfig.router).routeMessage(
+    (bool success, bytes memory returnData, ) = IRouter(s_dynamicConfig.router).routeMessage(
       Internal._toAny2EVMMessage(message, destTokenAmounts),
       Internal.GAS_FOR_CALL_EXACT_CHECK,
       message.gasLimit,
@@ -602,7 +602,7 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
       // Call the pool with exact gas to increase resistance against malicious tokens or token pools.
       // _callWithExactGas also protects against return data bombs by capping the return data size
       // at MAX_RET_BYTES.
-      (bool success, bytes memory returnData) = CallWithExactGas._callWithExactGas(
+      (bool success, bytes memory returnData, ) = CallWithExactGas._callWithExactGasSafeReturnData(
         abi.encodeWithSelector(
           pool.releaseOrMint.selector,
           originalSender,
@@ -613,8 +613,8 @@ contract EVM2EVMOffRamp is IAny2EVMOffRamp, AggregateRateLimiter, ITypeAndVersio
         ),
         address(pool),
         s_dynamicConfig.maxPoolReleaseOrMintGas,
-        Internal.MAX_RET_BYTES,
-        Internal.GAS_FOR_CALL_EXACT_CHECK
+        Internal.GAS_FOR_CALL_EXACT_CHECK,
+        Internal.MAX_RET_BYTES
       );
 
       // wrap and rethrow the error so we can catch it lower in the stack

contracts/src/v0.8/ccip/onRamp/EVM2EVMOnRamp.sol

@@ -351,6 +351,8 @@ contract EVM2EVMOnRamp is IEVM2AnyOnRamp, ILinkAvailable, AggregateRateLimiter,
     newMessage.messageId = Internal._hash(newMessage, i_metadataHash);
 
     // Emit message request
+    // Note this must happen after pools, some tokens (eg USDC) emit events that we
+    // expect to directly precede this event.
     emit CCIPSendRequested(newMessage);
     return newMessage.messageId;
   }

contracts/src/v0.8/ccip/pools/BurnFromMintTokenPool.sol

@@ -0,0 +1,33 @@
+// SPDX-License-Identifier: BUSL-1.1
+pragma solidity 0.8.19;
+
+import {ITypeAndVersion} from "../../shared/interfaces/ITypeAndVersion.sol";
+import {IBurnMintERC20} from "../../shared/token/ERC20/IBurnMintERC20.sol";
+
+import {TokenPool} from "./TokenPool.sol";
+import {BurnMintTokenPoolAbstract} from "./BurnMintTokenPoolAbstract.sol";
+
+/// @notice This pool mints and burns a 3rd-party token.
+/// @dev Pool whitelisting mode is set in the constructor and cannot be modified later.
+/// It either accepts any address as originalSender, or only accepts whitelisted originalSender.
+/// The only way to change whitelisting mode is to deploy a new pool.
+/// If that is expected, please make sure the token's burner/minter roles are adjustable.
+contract BurnFromMintTokenPool is BurnMintTokenPoolAbstract, ITypeAndVersion {
+  // solhint-disable-next-line chainlink-solidity/all-caps-constant-storage-variables
+  string public constant override typeAndVersion = "BurnFromMintTokenPool 1.2.0";
+
+  constructor(
+    IBurnMintERC20 token,
+    address[] memory allowlist,
+    address armProxy
+  ) TokenPool(token, allowlist, armProxy) {
+    // Some tokens allow burning from the sender without approval, but not all do.
+    // To be safe, we approve the pool to burn from the pool.
+    token.approve(address(this), type(uint256).max);
+  }
+
+  /// @inheritdoc BurnMintTokenPoolAbstract
+  function _burn(uint256 amount) internal virtual override {
+    IBurnMintERC20(address(i_token)).burnFrom(address(this), amount);
+  }
+}

contracts/src/v0.8/ccip/pools/BurnMintTokenPool.sol

@@ -2,17 +2,17 @@
 pragma solidity 0.8.19;
 
 import {ITypeAndVersion} from "../../shared/interfaces/ITypeAndVersion.sol";
-
 import {IBurnMintERC20} from "../../shared/token/ERC20/IBurnMintERC20.sol";
 
 import {TokenPool} from "./TokenPool.sol";
+import {BurnMintTokenPoolAbstract} from "./BurnMintTokenPoolAbstract.sol";
 
 /// @notice This pool mints and burns a 3rd-party token.
 /// @dev Pool whitelisting mode is set in the constructor and cannot be modified later.
 /// It either accepts any address as originalSender, or only accepts whitelisted originalSender.
 /// The only way to change whitelisting mode is to deploy a new pool.
 /// If that is expected, please make sure the token's burner/minter roles are adjustable.
-contract BurnMintTokenPool is TokenPool, ITypeAndVersion {
+contract BurnMintTokenPool is BurnMintTokenPoolAbstract, ITypeAndVersion {
   // solhint-disable-next-line chainlink-solidity/all-caps-constant-storage-variables
   string public constant override typeAndVersion = "BurnMintTokenPool 1.2.0";
 
@@ -22,37 +22,8 @@ contract BurnMintTokenPool is TokenPool, ITypeAndVersion {
     address armProxy
   ) TokenPool(token, allowlist, armProxy) {}
 
-  /// @notice Burn the token in the pool
-  /// @param amount Amount to burn
-  /// @dev The whenHealthy check is important to ensure that even if a ramp is compromised
-  /// we're able to stop token movement via ARM.
-  function lockOrBurn(
-    address originalSender,
-    bytes calldata,
-    uint256 amount,
-    uint64,
-    bytes calldata
-  ) external virtual override onlyOnRamp checkAllowList(originalSender) whenHealthy returns (bytes memory) {
-    _consumeOnRampRateLimit(amount);
+  /// @inheritdoc BurnMintTokenPoolAbstract
+  function _burn(uint256 amount) internal virtual override {
     IBurnMintERC20(address(i_token)).burn(amount);
-    emit Burned(msg.sender, amount);
-    return "";
-  }
-
-  /// @notice Mint tokens from the pool to the recipient
-  /// @param receiver Recipient address
-  /// @param amount Amount to mint
-  /// @dev The whenHealthy check is important to ensure that even if a ramp is compromised
-  /// we're able to stop token movement via ARM.
-  function releaseOrMint(
-    bytes memory,
-    address receiver,
-    uint256 amount,
-    uint64,
-    bytes memory
-  ) external virtual override whenHealthy onlyOffRamp {
-    _consumeOffRampRateLimit(amount);
-    IBurnMintERC20(address(i_token)).mint(receiver, amount);
-    emit Minted(msg.sender, receiver, amount);
   }
 }