fix(BOP-510): reject from == to in seizeWithMemo (self-seize) - #195
Open
rayyan224 wants to merge 3 commits into
Open
fix(BOP-510): reject from == to in seizeWithMemo (self-seize)#195rayyan224 wants to merge 3 commits into
rayyan224 wants to merge 3 commits into
Conversation
seizeWithMemo's balance move is a no-op when from == to, but the call still emitted Transfer, Memo, and Seized with no balance change once the address passed both the SeizeHolder and SeizeReceiver checks, letting a SEIZE_ROLE holder pollute the compliance trail. Reject from == to with InvalidReceiver before either policy check, mirroring the zero-receiver guard. Two existing fuzz tests (revert_accountNotBlocked, revert_ receiverPolicyForbids) lacked a from != to assumption and would have occasionally hit the new guard instead of their intended revert; added vm.assume(from != to) to keep them scoped to their own check. Extends the seize smoke journey with a self-seize edge case. Co-Authored-By: Claude <noreply@anthropic.com>
rayyan224
requested review from
amiecorso,
eric-ships,
ilikesymmetry and
stevieraykatz
as code owners
August 7, 2026 16:07
Interface Coverage✅ All interface functions have test coverage. |
📊 Forge Coverage (
|
| File | Lines | Stmts | Branches | Funcs |
|---|---|---|---|---|
| 🔴 B20FactoryLib.sol | 95.40% | 96.00% | 100.00% | 90.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 | 96.80% | 97.29% | 98.15% | 96.49% |
Full report: download artifact. To browse locally: make coverage (runs forge coverage + genhtml + opens the HTML report).
|
Comment on lines
+335
to
+343
| /// would be a burn), `from != to` (otherwise `_moveBalance` is a no-op that would still emit a | ||
| /// misleading `Transfer`/`Memo`/`Seized`), and — unlike a normal transfer — no sender/receiver/ | ||
| /// executor transfer policy is consulted, no allowance is spent, and `from` is not zero-checked | ||
| /// (consistent with the burn-blocked family; a zero/empty `from` fails the seizable or balance | ||
| /// check anyway). The membership checks are that `from` is blocked under `SEIZE_HOLDER_POLICY` | ||
| /// and `to` is authorized under `SEIZE_RECEIVER_POLICY` (mirroring `MINT_RECEIVER_POLICY`; an | ||
| /// unset slot is always-allow, so a treasury need not be allowlisted by default). Deliberately | ||
| /// does NOT reuse the factory-bootstrap privileged path (which would silently skip the receiver | ||
| /// policy); every skip here is explicit. |
Member
There was a problem hiding this comment.
Take a cleanup pass on this block. I know it's mostly legacy and not in scope for this PR but it could be slimmed down to about half the length imo.
Collaborator
Author
There was a problem hiding this comment.
High level, i think can be simplifed to:
Reassigns the balance of a user to a different user. Seized users are those whose SEIZE_HOLDER_POLICY alllowed policy returns true, SEIZE_RECEIVER_POLICY are those who can receive.
Collaborator
Author
There was a problem hiding this comment.
Trimmed it down per both suggestions — dropped the factory-bootstrap-path aside and folded the rest into a 3-line @dev.
Comment on lines
+64
to
+65
| // `account` is seizable (blocked) and SEIZE_RECEIVER_POLICY is unset (always-allow), so both | ||
| // membership checks would pass — the self-seize guard must fire before either is consulted. |
Member
There was a problem hiding this comment.
Confusing inline honestly... consider removing.
Collaborator
Author
There was a problem hiding this comment.
Removed — agreed, the @notice above already covers it.
Co-authored-by: katzman <steve.katzman@coinbase.com>
- IB20.sol: collapse the two-line InvalidReceiver @dev back to one line (accepted suggestion). - MockB20.sol: cut the seizeWithMemo @dev block roughly in half per review feedback; legacy verbosity, not specific to this change. - seizeWithMemo.t.sol: drop the inline comment in test_seizeWithMemo_revert_selfSeize; the @notice above the function already covers it. Co-Authored-By: Claude <noreply@anthropic.com>
robriks
approved these changes
Aug 7, 2026
robriks
left a comment
robriks
left a comment
Collaborator
There was a problem hiding this comment.
good stuff!
surfaced a side question on the rust PR about 0-value seizures, I'll run it by Roger
stevieraykatz
approved these changes
Aug 7, 2026
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
3 participants
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
seizeWithMemo's balance move is a no-op whenfrom == to, but the call still emittedTransfer,Memo, andSeizedwith no balance change once the address passed both theSeizeHolderandSeizeReceiverchecks — letting aSEIZE_ROLEholder pollute the compliance trail with misleading events. No fund loss.from == towithInvalidReceiverinMockB20.seizeWithMemo(shared by the asset and stablecoin mocks), right after the existing zero-receiver check.IB20.seizeWithMemonatspec with the new revert condition.test_seizeWithMemo_revert_selfSeizeand two revert-order tests (role_beats_selfSeize,selfSeize_beats_blocked); fixes two existing fuzz tests (revert_accountNotBlocked,revert_receiverPolicyForbids) that lacked afrom != toassumption and would occasionally hit the new guard instead of their intended revert.seizesmoke journey (script/smoke/journeys/seize.py) with a self-seize edge case.Linear
BOP-510 — Cantina finding L-04.
Test plan
forge test— all 724 tests pass (0 failed).forge test --match-contract B20SeizeWithMemo— 22/22 pass, including the new self-seize and revert-order tests.forge fmt --checkclean on all changed files.