Add ability for proxy and versioned verifier resolver to withdraw fee… #1511

crispymangoes · 2025-12-23T19:43:36Z

Allows Proxy.sol to withdraw tokens from it
Allows VersionedVerifierResolver to withdraw tokens from it
Adds a zero address check to the fee token handler lib.
Changes MAX_GAS_BUFFER_TO_UPDATE_STATE into a variable that can be changed

… tokens.

RensR · 2025-12-23T21:23:32Z

chains/evm/contracts/offRamp/OffRamp.sol


+  /// @notice Gas buffer to update state.
+  // Example, 5k for updating the state + 5k for the event and misc costs.
+  uint64 internal s_maxGasBufferToUpdateState;


probably fine as uint32 right?

Yeah I think that will work even for chains that have wild opcode prices. Fixed 4be9538

RensR · 2025-12-29T13:16:14Z

chains/evm/contracts/offRamp/OffRamp.sol


    uint256 gasLeft = gasleft();
-    if (gasLeft <= MAX_GAS_BUFFER_TO_UPDATE_STATE) {
+    if (gasLeft <= s_maxGasBufferToUpdateState) {


caching this like
uint256 gasBuffer = s_maxGasBufferToUpdateState;

saves 136 gas

Oh nice catch, fixed! 4be9538

…into fix-executor-proxy-and-resolver-fee-handling

0xsuryansh · 2025-12-29T17:28:05Z

chains/evm/contracts/offRamp/OffRamp.sol

  }

+  /// @notice Returns the max gas buffer to update state.
+  function getmaxGasBufferToUpdateState() external view virtual returns (uint32) {


Suggested change

function getmaxGasBufferToUpdateState() external view virtual returns (uint32) {

function getMaxGasBufferToUpdateState() external view virtual returns (uint32) {

Ah good catch!

Fixed 28192ad

…into fix-executor-proxy-and-resolver-fee-handling

RensR · 2025-12-30T10:20:06Z

chains/evm/contracts/offRamp/OffRamp.sol


+  /// @notice Gas buffer to update state.
+  // Example, 5k for updating the state + 5k for the event and misc costs.
+  uint32 internal s_maxGasBufferToUpdateState;


Let's make it immutable to save the setting functions and the gas for reading. That allows us to set it to a different value for chains with strange gas behaviour, and changing gas costs are extremely rare. With 1.7, swapping out a single offramp is possible so even if we need to deploy a new one it should be fine.

…into fix-executor-proxy-and-resolver-fee-handling

kylesmartin · 2025-12-30T22:36:12Z

chains/evm/contracts/Proxy.sol

+  }
+
+  /// @dev Internal method that allows for reuse in constructor.
+  function _setFeeAggregator(


Maybe we can move this fee aggregator code to the FeeTokenHandler? Since it is duplicated

Cant do storage changes in a lib

RensR · 2026-01-02T13:57:42Z

chains/evm/contracts/test/e2e/e2e.feeWithdraw.t.sol

+
+  /// @notice Test fee withdrawal from all components after ccipSend
+  function test_FeeWithdrawal_AfterCcipSend() public {
+    vm.pauseGasMetering();


This isn't a gas test so no need to pause/resume gas metering

chains/evm/contracts/test/e2e/e2e.feeWithdraw.t.sol

RensR · 2026-01-02T13:58:31Z

chains/evm/contracts/test/e2e/e2e.feeWithdraw.t.sol

+      data: "",
+      tokenAmounts: new Client.EVMTokenAmount[](1),
+      feeToken: s_sourceFeeToken,
+      extraArgs: ExtraArgsCodec._encodeGenericExtraArgsV3(


Can just provide "" instead

RensR · 2026-01-02T13:59:44Z

chains/evm/contracts/test/e2e/e2e.feeWithdraw.t.sol

+      uint256 actualOnRampBalance = IERC20(s_sourceFeeToken).balanceOf(address(s_onRamp));
+
+      // Only proceed if there's actually a balance to withdraw
+      if (actualOnRampBalance > 0) {


You already checked that it's greater than before, so it must be non-zero

RensR · 2026-01-02T14:00:32Z

chains/evm/contracts/test/e2e/e2e.feeWithdraw.t.sol

+      // Only proceed if there's actually a balance to withdraw
+      if (actualOnRampBalance > 0) {
+        uint256 aggregatorBalanceBeforeOnRamp = IERC20(s_sourceFeeToken).balanceOf(s_feeAggregator);
+        vm.stopPrank();


We can do this prank switch once before we do any of these withdrawals

RensR · 2026-01-02T14:01:20Z

chains/evm/contracts/test/e2e/e2e.feeWithdraw.t.sol

+
+      // Only proceed if there's actually a balance to withdraw
+      if (actualOnRampBalance > 0) {
+        uint256 aggregatorBalanceBeforeOnRamp = IERC20(s_sourceFeeToken).balanceOf(s_feeAggregator);


Can keep this between contracts so we don't have to do a pre-post for every one, just a post and we have a pre already. Just update the pre to the post after each contract is checked

RensR · 2026-01-02T14:01:55Z

chains/evm/contracts/test/e2e/e2e.feeWithdraw.t.sol

+        // Use actual balance withdrawn, not the calculated difference
+        uint256 actualWithdrawn = aggregatorBalanceAfterOnRamp - aggregatorBalanceBeforeOnRamp;
+        assertEq(actualWithdrawn, actualOnRampBalance, "OnRamp fees should be withdrawn to aggregator");
+        totalFeesWithdrawn += actualWithdrawn;


Check against receipts?

RensR · 2026-01-02T14:02:38Z

chains/evm/contracts/test/e2e/e2e.feeWithdraw.t.sol

+      uint256 poolFeeBalance = end.tokenPoolBalance - initial.tokenPoolBalance;
+
+      // If pool received fees from the transaction, test withdrawing them
+      // Otherwise, manually add fees to test the withdrawal mechanism


We can just ensure it always has fees in the setup no? This should be deterministic

RensR · 2026-01-02T14:03:42Z

chains/evm/contracts/test/e2e/e2e.feeWithdraw.t.sol

+  }
+
+  /// @notice Test that verifier fees go to resolver (proxy), not implementation
+  function test_VerifierFeeIssue_FeesGoToResolverNotImplementation() public {


Don't you already test this in the test above? Can probably remove this one

Oh yeah you are right, I think this test came about when I was confirming it was an issue, but the main test is sufficiently covering this .

RensR · 2026-01-02T14:03:55Z

chains/evm/contracts/test/e2e/e2e.feeWithdraw.t.sol

+  }
+
+  /// @notice Test PAL compatibility: permissionless vs onlyOwner
+  function test_PALCompatibility_PermissionlessVsOnlyOwner() public {


You already test this in the first test

Oh yeah good point, will remove

RensR · 2026-01-02T14:04:33Z

chains/evm/contracts/test/offRamp/OffRamp/OffRamp.constructor.t.sol

 import {BaseTest} from "../../BaseTest.t.sol";

 contract OffRamp_constructor is BaseTest {
+  uint32 internal constant DEFAULT_MAX_GAS_BUFFER_TO_UPDATE_STATE = 5000 + 5000 + 2000;


This is duplicated a few places

…into fix-executor-proxy-and-resolver-fee-handling

…red lib which reverts if recipient is zero address

RensR · 2026-01-02T17:56:28Z

chains/evm/contracts/test/e2e/e2e.feeWithdraw.t.sol

+
+import {TokenPoolHelper} from "../helpers/TokenPoolHelper.sol";
+import {MockVerifier} from "../mocks/MockVerifier.sol";
+


nit: rm newline here and line 15

RensR · 2026-01-02T17:56:49Z

chains/evm/contracts/test/e2e/e2e.feeWithdraw.t.sol

+
+import {OnRampSetup} from "../onRamp/OnRamp/OnRampSetup.t.sol";
+
+import {BurnMintERC20} from "@chainlink/contracts/src/v0.8/shared/token/ERC20/BurnMintERC20.sol";


this is first party, not third party. It should go in the higher section

github-actions · 2026-01-03T00:38:10Z

Metric	`fix-executor-proxy-and-resolver-fee-handling`	`develop`
Coverage	69.4%	69.0%

Add ability for proxy and versioned verifier resolver to withdraw fee…

6aa3f71

… tokens.

crispymangoes requested a review from a team as a code owner December 23, 2025 19:43

crispymangoes added 2 commits December 23, 2025 12:18

Make MAX_FEE_TO_UPDATE_STATE into a state variable and update bindings

f614f20

Fix solhint erorrs

b27ed5c

RensR reviewed Dec 29, 2025

View reviewed changes

crispymangoes added 4 commits December 29, 2025 07:32

Small gas savings and slot space savings fix

4be9538

Merge branch 'develop' of github.com:smartcontractkit/chainlink-ccip …

378a44d

…into fix-executor-proxy-and-resolver-fee-handling

Merge branch 'develop' of github.com:smartcontractkit/chainlink-ccip …

ba7a256

…into fix-executor-proxy-and-resolver-fee-handling

Merge branch 'develop' of github.com:smartcontractkit/chainlink-ccip …

a5c1e1f

…into fix-executor-proxy-and-resolver-fee-handling

0xsuryansh reviewed Dec 29, 2025

View reviewed changes

crispymangoes added 2 commits December 29, 2025 11:09

Fix typo

28192ad

Merge branch 'develop' of github.com:smartcontractkit/chainlink-ccip …

0500e49

…into fix-executor-proxy-and-resolver-fee-handling

kylesmartin previously approved these changes Dec 29, 2025

View reviewed changes

RensR reviewed Dec 30, 2025

View reviewed changes

crispymangoes added 5 commits December 30, 2025 08:14

Make maxGasBufferToUpdateState immutable instead of mutable.

22b5348

Make withdrawFeeTokens permissionless

6dc7035

Cherry pick e2e fee withdraw test into this branch

0d19432

Merge branch 'develop' of github.com:smartcontractkit/chainlink-ccip …

68da0b5

…into fix-executor-proxy-and-resolver-fee-handling

Fix solhint error and generate go bindings

cec0301

crispymangoes dismissed kylesmartin’s stale review via cec0301 December 30, 2025 19:06

Remove unused imports

107207b

kylesmartin reviewed Dec 30, 2025

View reviewed changes

RensR reviewed Jan 2, 2026

View reviewed changes

chains/evm/contracts/test/e2e/e2e.feeWithdraw.t.sol Show resolved Hide resolved

RensR reviewed Jan 2, 2026

View reviewed changes

crispymangoes added 7 commits January 2, 2026 08:16

Address test comments

1ff4e17

Merge branch 'develop' of github.com:smartcontractkit/chainlink-ccip …

7f4ca8d

…into fix-executor-proxy-and-resolver-fee-handling

Remove redundant zero address check

1a35e42

Generate go bindings

e9a9dbd

Fix solhint errors

1b19a07

Remove zero address checks for fee aggregator as they all use the sha…

9affe7f

…red lib which reverts if recipient is zero address

Fix unused imports

d7c0493

RensR reviewed Jan 2, 2026

View reviewed changes

crispymangoes added 2 commits January 2, 2026 10:08

Fix import nits and remove redundant pranks

d49e4ba

Remove missed redundant vm prank calls

f607ba3

RensR approved these changes Jan 3, 2026

View reviewed changes

	function getmaxGasBufferToUpdateState() external view virtual returns (uint32) {
	function getMaxGasBufferToUpdateState() external view virtual returns (uint32) {


		import {TokenPoolHelper} from "../helpers/TokenPoolHelper.sol";
		import {MockVerifier} from "../mocks/MockVerifier.sol";


		import {OnRampSetup} from "../onRamp/OnRamp/OnRampSetup.t.sol";

		import {BurnMintERC20} from "@chainlink/contracts/src/v0.8/shared/token/ERC20/BurnMintERC20.sol";

Add ability for proxy and versioned verifier resolver to withdraw fee… #1511

Are you sure you want to change the base?

Add ability for proxy and versioned verifier resolver to withdraw fee… #1511

Uh oh!

Conversation

crispymangoes commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RensR Jan 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 3, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

crispymangoes commented Dec 23, 2025 •

edited

Loading

RensR Jan 2, 2026 •

edited

Loading