fix(cash): keep a module's authority over its own pending withdrawal after de-whitelisting

[simonedamico]

Risk this closes

When governance removed a withdraw-requesting module from CashModule's withdraw-requester list or from the DataProvider whitelist while one of its withdrawal holds was pending, CashModule stopped treating that hold as the module's:

• anyone could processWithdrawal the hold into the module, skipping the module's own bookkeeping. For PaymentLinkModule (draft [PAY-1170] Add PaymentLinkModule and deploy scripts #349) the sender's funds land as unrecorded escrow surplus, recoverable only partly by rescueSurplus when the token is in a shortfall;
• the module could no longer cancelWithdrawalByModule its own hold;
• cancelOldWithdrawal skipped the cancelBridgeByCashModule callback, so the module's record went stale.

This affects StockWithdrawModule today and PaymentLinkModule once deployed. The Codex review of #349 found it; #349 mitigated it with rescueSurplus and a governance rule (never de-whitelist with live Requested links), and left the real fix to CashModule. This PR is that fix. The retirement preflight in RetireSupersededModules.s.sol guards the same hazard operationally ("processWithdrawal is open to anyone").

Change

• requestWithdrawalByModule records the request as the module's own: SafeCashConfig.withdrawalRequestedByModule, set in _requestWithdrawal, cleared wherever the pending request is deleted (process, cancel).
• CashLendLib.isModuleWithdrawal(safe) = record, or recipient on the requester list (the second half keeps today's rules for requests without the record). _processWithdrawal, owner cancelWithdrawal and cancelWithdrawalByModule use it, so removal blocks new requests only:
  • only the recipient module can process its recorded hold;
  • the module can still cancel it (the DataProvider check in cancelWithdrawalByModule now applies only to requests without the record);
  • owners still cannot cancelWithdrawal a module's request directly (unchanged rule); they cancel through the module, replace it with their own requestWithdrawal, or a spend cancels it.
• cancelOldWithdrawal:
  • module on both whitelists: callback exactly as before, a revert still reverts the cancel;
  • removed module with a recorded request: _notifyRemovedModule, a call capped at 200k gas whose result is ignored and whose return data is not copied. A removed module may be broken or retired to a reverting implementation, and spends, preLiquidate and owner replacements all cancel through here, so it must not be able to block them. Removing a module from the DataProvider used to stop its callback from running at all; now the callback still runs (with only the authority the module already holds over its own request) but can no longer block the cancel. Measured in a later transaction than the request: StockWithdrawModule's callback 68,160 gas, PaymentLinkModule's 46,190 gas; tests keep both under half the cap. Whoever submits a cancel picks its gas limit but cannot starve the callback while the cancel succeeds: starving it leaves the outer call 1/64 of the gas, too little to finish. Tests sweep the outer gas limit of a relayed owner replacement in 1k steps from far below the cheapest successful call to 100k above it, against a mock and against the real StockWithdrawModule (and PaymentLinkModule, see below), and require every success to have delivered the callback.

Requests placed before the upgrade carry no record and keep the current-membership rules until they clear, so the "no live holds" preflight still applies to modules removed while such holds exist.

Storage layout (deploy-proxy rule 5)

Both impls keep everything in ERC-7201 namespaces; forge inspect … storageLayout of CashModuleCore and CashModuleSetters is empty before and after. The namespaced CashModuleStorage was diffed through a probe contract holding it as a plain state variable (forge inspect, compiler AST ids stripped), master vs this branch:

 t_struct(SafeCashConfig) numberOfBytes=576
 ...
   slot 17 off  0 incomingMode t_enum(Mode)
   slot 17 off  1 lendOptedOut t_bool
   slot 17 off  2 lendOptOutFinalizeTime t_uint96
   slot 17 off 14 usesLendGateway t_bool
+  slot 17 off 15 withdrawalRequestedByModule t_bool

No other line changes: one bool appended at the end of SafeCashConfig (a mapping value), packed into its existing last slot, struct size unchanged, WithdrawalRequest and SafeData (the getData ABI) untouched. test_record_packsIntoSafeCashConfigTailSlot pins the slot and offset and checks that writing the record leaves usesLendGateway intact.

Upgrade path (not done here; nothing deployed)

CashLendLib is a linked library, so as in DeployCashRepayFixProd.s.sol: deploy a new CashLendLib (forge links it) and new CashModuleCore and CashModuleSetters implementations via CREATE3 with fresh salts, then a 2-day upgrade-timelock batch: CashModule.upgradeToAndCall(core, "") and setCashModuleSettersAddress(setters), with a verify script per the deploy-proxy rules. Both calls must land in the same batch, with both impls linked to the new library: a mixed old/new pair can create requests without the record or leave a stale one. Holds placed before the upgrade carry no record, so a module should be removed only once its pre-upgrade holds have cleared (the existing preflight check). test_verifyBytecode_CashModuleCore / _CashModuleSetters in VerifyOPMainnet.t.sol are skipped (both pass on master) and should be re-enabled once that upgrade is live, the same way as the EtherFiSafeFactory skip.

Size: CashModuleCore runtime 23,806 → 23,883 B (693 B margin), CashModuleSetters 23,130 → 23,226 B.

Tests

New test/safe/modules/cash/ModuleWithdrawalAuthority.t.sol (mock module, 29 tests): requester list only, DataProvider only and both orders for processing and module cancel; owners' direct cancel still rejected; owner replacement and card spend notify the removed module; reverting callbacks cannot block replacement, spends or preLiquidate; a gas-burning callback is capped; the outer gas limit cannot starve the callback; a whitelisted module's reverting callback still reverts the cancel; record lifecycle (process, cancel, zero delay, owner request) and slot; requests without the record follow today's rules.

test/stock-withdraw/StockWithdrawModule.t.sol: both removal orders, only the module processes the hold and executeWithdrawal still bridges; the expired-order release and the owners' replacement clear the order in both orders; requester-list removal blocks new requests and keeps the owner-signed cancel working. A separate contract with the order and the removal persisted in setUp checks the callback fits the cap and runs the gas-limit sweep (the shared setup and helpers moved into an abstract StockWithdrawModuleTestBase). Mutations: a 20k cap fails 4 tests in the authority suite, a 40k cap fails both StockWithdrawModule callback tests.

Withdrawals.t.sol: the test that pinned the old DataProvider-removal revert now asserts the module can still cancel.

Run locally on an Optimism fork (TEST_CHAIN=10 ENV=mainnet TEST_RPC=https://optimism-rpc.publicnode.com, plus OPTIMISM_RPC for the suites that read it, -j 4):

• test/safe/modules/cash/** (default profile): 452 passed, 0 failed
• test/stock-withdraw/StockWithdrawModule.t.sol: 59 passed
• FOUNDRY_PROFILE=lend, test/safe/modules/cash/lend/{CashLendOptOut,StargateGateway,EnsoSwapGateway,AcrossSwapGateway,Spend,RepaySourcing}.t.sol: 131 passed
• test/upgrade-bytecode-verification/VerifyOPMainnet.t.sol: 40 passed, 4 skipped (2 existing, 2 above)

Not run locally: the rest of the rest and deployed CI shards and the other lend suites.

PaymentLinkModule (#349)

#349's tests pin the old CashModule behaviour (third-party processing works, DataProvider-removed cancel reverts), so they fail against this change. Branch claude/plm-tests-on-cashmodule-fix (no PR) is #349 with this branch merged in and test/payment-link/PaymentLinkModule.t.sol updated: both removal orders and single-list removals (no third-party processing, fundLink and claim work, owners' cancel releases the hold, card spends and owner withdrawals still deliver the callback, new links blocked, callback fits the cap and survives the gas-limit sweep with the hold persisted in setUp). The tests that used de-whitelisting to make CashModule skip the callback, and the three hazard tests, now model a hold placed before the upgrade (no record), where the old rules and #349's governance rule still apply. test/payment-link/* there: 101 passed. Whichever of #349 and this PR merges second carries those test changes. #349's governance-rule docs stay correct for pre-upgrade holds; they can be scoped down once this upgrade is live.

Review

Codex (codex exec -s read-only, adversarial), two passes: READY both times, no Blocking findings.

• Pass 1, should fix: the callback gas check deleted storage written in the same test transaction (cheaper than persisted state), and an outer gas limit sweep was missing. Fixed: the measurement runs in a later transaction than the request, and the starvation tests were added.
• Pass 1, should fix: a failed callback can strand StockWithdrawModule's order record (it has no reconcile). Not fixed here (module change, below). Before this PR the callback was always skipped after a DataProvider removal, so the record was always stranded then.
• Pass 1, should fix: requests without the record keep the old hazard, and a mixed Core/Setters upgrade could create unrecorded requests or stale records. Covered under Upgrade path.
• Pass 2, should fix: a binary search alone assumes monotonic success, and the mock is not the real module. Fixed: linear 1k-step sweep, run against the real StockWithdrawModule and PaymentLinkModule with persisted fixtures.

Follow-ups (not in this PR)

• StockWithdrawModule's owner-signed cancelWithdrawal takes its nonce from safe.useNonce(), which only serves enabled modules, so after a DataProvider removal its owners use cancelExpiredWithdrawal or a replacement withdrawal instead (tested).
• StockWithdrawModule reconcile (clear an order whose CashModule hold is gone), so a missed callback cannot strand the order.
• Deploy and verify scripts for the CashModule upgrade.
• After the upgrade is live: re-enable the two VerifyOPMainnet CashModule checks and scope [PAY-1170] Add PaymentLinkModule and deploy scripts #349's governance-rule docs to pre-upgrade holds.

🤖 Generated with Claude Code

---

Note

Medium Risk
Changes CashModule withdrawal authorization and introduces capped external callbacks to de-whitelisted modules; incorrect logic could strand or mis-route reserved funds, though behavior is heavily tested and scoped to module-initiated holds.

Overview
Fixes a gap where de-whitelisting a withdraw-requesting module (from CashModule’s requester list or DataProvider) could strip it of control over an already pending hold: third parties could processWithdrawal into the module, the module could not cancelWithdrawalByModule, and cancelBridgeByCashModule was skipped.

requestWithdrawalByModule now sets a per-request flag withdrawalRequestedByModule on SafeCashConfig (packed into the existing tail slot). CashLendLib.isModuleWithdrawal treats a hold as module-owned when that flag is set or when the recipient is still on the requester list (legacy / pre-upgrade requests). Process, owner cancel, and module cancel paths use this helper so removal only blocks new module requests, not lifecycle of recorded holds.

When a competing cancel clears a module hold, cancelOldWithdrawal still requires a whitelisted module’s callback to succeed; for removed modules with the record, _notifyRemovedModule invokes cancelBridgeByCashModule with a 200k gas cap and ignores success/revert so spends, liquidations, and owner replacement withdrawals cannot be blocked. The flag is cleared whenever the pending request is deleted.

Adds ModuleWithdrawalAuthority.t.sol, extends StockWithdrawModule and Withdrawals tests, and temporarily skips OP bytecode verification for CashModule impls until upgrade.

^{Reviewed by Cursor Bugbot for commit 551fc55. Bugbot is set up for automated code reviews on this repo. Configure here.}

---

^{Need help on this PR? Tag @codesmith-bot with what you need.}

[certora-run]

Verification Results

• Group ID: a49ffa13-1154-48c4-a1a7-773e978cb63f
• Commit: 551fc55

Job	Result	VERIFIED
EtherFiSafe.…	✅	14
CashModuleCo…	✅	2