feat(policy): enforce TRANSFER_EXECUTOR_POLICY on every transfer path - #224
Open
rayyan224 wants to merge 3 commits into
Open
feat(policy): enforce TRANSFER_EXECUTOR_POLICY on every transfer path#224rayyan224 wants to merge 3 commits into
rayyan224 wants to merge 3 commits into
Conversation
Interface Coverage✅ All interface functions have test coverage. |
📊 Forge Coverage (
|
| File | Lines | Stmts | Branches | Funcs |
|---|---|---|---|---|
| 🟡 B20FactoryLib.sol | 97.70% | 98.00% | 100.00% | 95.00% |
| 🔴 test/lib/ForceFeeder.sol | 0.00% | 0.00% | 100.00% | 0.00% |
| 🔴 test/lib/PrecompileProbe.sol | 0.00% | 0.00% | 0.00% | 0.00% |
| 🟢 MockActivationRegistry.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟢 MockActivationRegistryStorage.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟢 MockB20.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟢 MockB20Asset.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟡 MockB20Factory.sol | 98.96% | 99.10% | 100.00% | 100.00% |
| 🟢 MockB20Stablecoin.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟢 MockB20Storage.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| 🟡 MockPolicyRegistry.sol | 100.00% | 99.54% | 97.67% | 100.00% |
| 🟢 MockPolicyRegistryStorage.sol | 100.00% | 100.00% | 100.00% | 100.00% |
| Total | 97.05% | 97.49% | 98.12% | 97.00% |
Full report: download artifact. To browse locally: make coverage (runs forge coverage + genhtml + opens the HTML report).
|
rayyan224
force-pushed
the
feat/policy-transfer-executor-enforcement
branch
from
September 10, 2026 20:11
8b64ef2 to
2c63f86
Compare
Centralize the executor gate in `_transfer` so TRANSFER_EXECUTOR_POLICY is checked against `msg.sender` on all four transfer entrypoints (`transfer`, `transferFrom`, `transferWithMemo`, `transferFromWithMemo`), including when `msg.sender == from`. Previously it ran only on the delegated `transferFrom` paths and only when `msg.sender != from`. This closes two bypasses that made an executor allowlist unenforceable: direct `transfer` was never gated, and a self-`transferFrom` skipped the check. The executor scope is now a true "who may initiate a transfer" gate. Behavioral only — no new selectors, events, errors, or storage. An unset executor slot stays always-allow, so tokens that never configured the policy are unaffected. The factory bootstrap bypass and allowance accounting are unchanged. - MockB20: move executor check into `_transfer` (first, before sender/receiver); drop the duplicated body checks and the `msg.sender == from` carve-out - tests: executor cases on transfer + memo, EXECUTOR woven into the revert-order suites, and the old self-caller skip test inverted to pin the closed loophole - interface/comments: IB20 natspec, mock/storage comments Co-Authored-By: Claude <noreply@anthropic.com>
rayyan224
force-pushed
the
feat/policy-transfer-executor-enforcement
branch
from
September 10, 2026 20:28
2c63f86 to
6a55bf8
Compare
rayyan224
commented
Sep 10, 2026
| // is still not decremented. The executor policy is enforced centrally | ||
| // in `_transfer` (on `msg.sender`), which honors the bootstrap bypass. | ||
| _consumeAllowance(from, msg.sender, amount); | ||
| if (!_isPrivileged() && msg.sender != from) { |
Collaborator
Author
There was a problem hiding this comment.
Removed here, becaues repulled in the helper no need for an extra SLOAD
rayyan224
marked this pull request as ready for review
September 10, 2026 20:29
rayyan224
requested review from
amiecorso,
eric-ships,
ilikesymmetry and
stevieraykatz
as code owners
September 10, 2026 20:30
Record that TRANSFER_EXECUTOR_POLICY now gates every transfer path, including memo variants, so integrators can see the new revert order and coverage. Co-authored-by: Cursor <cursoragent@cursor.com>
Add a task-oriented guide for gating TRANSFER_EXECUTOR_POLICY with a transfer-agent example, and index it in docs/README.md. Co-Authored-By: Claude <noreply@anthropic.com>
rayyan224
commented
Sep 11, 2026
|
|
||
| ## Motivation | ||
|
|
||
| An issuer may want an executor allowlist: only specific, approved contracts or accounts may initiate a transfer, for example a settlement contract that moves tokens on a holder's behalf. `TRANSFER_EXECUTOR_POLICY` exists for this, but the previous scope could not enforce it consistently with `TRANSFER_SENDER_POLICY` and `TRANSFER_RECEIVER_POLICY`, which already run on every transfer path. |
Collaborator
Author
There was a problem hiding this comment.
Talk about transfer agent, here
rayyan224
commented
Sep 11, 2026
|
|
||
| This change makes `TRANSFER_EXECUTOR_POLICY` apply to every transfer path. The executor gate now checks `msg.sender` on `transfer`, `transferFrom`, `transferWithMemo`, and `transferFromWithMemo`, including when `msg.sender == from`. Previously the check ran only on the delegated `transferFrom` paths, and only when `msg.sender != from`. | ||
|
|
||
| The change is purely behavioral. It adds no new selectors, events, errors, or storage. A token that never sets `TRANSFER_EXECUTOR_POLICY` keeps the unset always-allow default, so it is unaffected. |
Collaborator
Author
There was a problem hiding this comment.
Make sure to talk about the breaking change in the summary
rayyan224
commented
Sep 11, 2026
| When more than one check would fail, the caller sees the first revert in that order: | ||
|
|
||
| There are no new storage slots. The `TRANSFER_EXECUTOR_POLICY` policy ID is read from the same packed slot as before; `_transfer` now reads all three transfer-side policy IDs from that slot in one `SLOAD` instead of the executor lane being pre-warmed by a separate read in `transferFrom`'s body. | ||
|
|
rayyan224
commented
Sep 11, 2026
|
|
||
| ```mermaid | ||
| flowchart TD | ||
| subgraph beforeTransfer ["Before: transfer / transferWithMemo"] |
Collaborator
Author
There was a problem hiding this comment.
Check to move everything in the helper.
rayyan224
commented
Sep 11, 2026
| // check. Solidity emits a single SLOAD for the struct read + | ||
| // masked extracts for the named fields. | ||
| MockB20Storage.TransferPolicyIds memory packed = MockB20Storage.layout().transferPolicyIds; | ||
| if (!IPolicyRegistry(POLICY_REGISTRY).isAuthorized(packed.executor, msg.sender)) { |
Collaborator
Author
There was a problem hiding this comment.
Add optimization for, cache
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Makes
TRANSFER_EXECUTOR_POLICYapply to every transfer path. The executor gate now checksmsg.senderontransfer,transferFrom,transferWithMemo, andtransferFromWithMemo— including whenmsg.sender == from. Previously it ran only on the delegatedtransferFrompaths, and only whenmsg.sender != from.This targets the Q4 "Denim" candidate "Apply transfer executor policy on normal transfer" (P2) — letting issuers use an executor allowlist to restrict who may initiate a transfer (e.g. only an approved settlement contract).
Why
The old behavior left the executor scope unenforceable as an initiator gate, via two bypasses:
transferwas never gated — the initiator ismsg.sender(==from), and the check ran only insidetransferFrom.transferFromskipped the check —msg.sender == frombypassed it, so a non-allowlisted holder could routetransferFrom(self, to, amount)to move tokens anyway.Centralizing the check in
_transferonmsg.senderand removing themsg.sender == fromcarve-out closes both.Approach
test/lib/mocks/MockB20.sol): executor check moved into_transfer(first, before sender/receiver, under the existing_isPrivileged()bootstrap bypass); duplicated body checks and themsg.sender == fromcarve-out removed. Allowance is still consumed in thetransferFrom*bodies first, so revert order is unchanged.transfer.t.sol+ memo parity; EXECUTOR woven intotransfer_revertOrder.t.sol(C(7,2)=21 pairs) and the memo sequential order test; the oldtransferFromself-caller skip test inverted intotest_transferFrom_revert_selfCaller_executorPolicyForbidsto pin the closed loophole.IB20.solnatspec for the executor scope + thetransfer/transferFromrevert lists; stale "delegated-only" comments in the mock/storage/revert-order headers.Compatibility
Purely behavioral — no new selectors, events, errors, or storage. An unset executor slot stays always-allow, so tokens that never configured the policy are unaffected. Factory bootstrap bypass and allowance accounting are unchanged.
Breaking only for a token that has set a restrictive
TRANSFER_EXECUTOR_POLICYand relies on holders moving their own tokens viatransfer/ self-transferFrom— those holders must now be authorized as initiators.Testing
forge test— 746 passed, 0 failed, 4 skipped (pre-existing mock-only privileged skips).🤖 Generated with Claude Code