code/cl8y-bridge-monorepo

Fork 0

security(evm): reject fee-on-transfer ERC20 deposits that deliver less than recorded #189

New issue

Open

opened 2026-09-12 13:07:44 +00:00 by PlasticDigits · 0 comments

PlasticDigits commented

2026-09-12 13:07:44 +00:00

Owner

Summary

EVM Bridge.depositERC20 computes netAmount = amount - fee, optionally safeTransferFroms the fee to feeConfig.feeRecipient, then safeTransferFroms netAmount to LockUnlock. Neither transfer measures balanceOf before/after. The deposit record, Deposit event, and xchainHashId all use the requested netAmount, not tokens actually received.

LockUnlock.unlock already enforces exact deltas (InvalidUnlockThis / InvalidUnlockTo). MintBurn.burn / mint do the same. Lock deposits do not. Docs already list fee-on-transfer as unsupported (OPERATIONAL_NOTES.md §4). TokenRegistry.registerToken is owner-only and does not probe transfer semantics.

This is not #179 (native recoverAsset vs depositNative liability). This is not #178 (unset guardBridge / rateLimitBridge). This is not #100 (protocol fee vs rate-limit minimums). This is not #9 (frontend displayed amount). Keyword overlap on fee / depositERC20 / netAmount is not this bug.

Internal review id: EVM-M1. Still in source as of 2026-09-12.

Bundle (same ticket, do not split):

Measure lock-side (and fee) ERC-20 balance deltas on depositERC20; revert if received ≠ expected. Do not credit a reduced amount (that would change hash inputs).
Fail closed at TokenRegistry.registerToken for non-standard transfer semantics (owner-funded 1-unit round-trip probe, or equivalent).
Wire existing MockTransferTaxToken (10% tax, currently unreferenced by any test) through deposit + registration + unlock fixtures.

Founder-required contracts. No community autoland. Do not add ready.

Impact (today vs hypothetical)

Inventory / dest-credit invariant is broken in source today whenever a fee-on-transfer (or otherwise deflationary-on-transfer) ERC-20 is registered as LockUnlock. depositERC20 will record and hash netAmount while LockUnlock holds less. Destination mint/unlock of the recorded amount then over-credits relative to source inventory (mint path) or cannot be backed 1:1. Other lockers of the same token are short.

Registration is onlyOwner, and §4 already says FoT is unsupported. That is an operational hope, not an invariant. Listing any tax/deflationary token (mistaken economic-token add, or a token that later turns on a fee) activates the hole with no on-chain reject.

Hypothetical-only if every registered lock token is guaranteed standard transfer/transferFrom forever. Source still allows registering and depositing non-standard tokens.

Do not publish a mainnet FoT deposit sequence, token-tax configuration, or live RPC probe scripts.

EVM: `depositERC20` records requested net, not received

depositERC20 (whenNotPaused, nonReentrant):

Requires registered token, dest chain, destToken != 0, amount > 0.
fee = calculateFee(msg.sender, amount); netAmount = amount - fee.
If fee > 0: safeTransferFrom(msg.sender, feeRecipient, fee) with no balanceOf(feeRecipient) delta check.
Then safeTransferFrom(msg.sender, address(lockUnlock), netAmount) with no balanceOf(LockUnlock) delta check.
Stores deposits[xchainHashId].amount = netAmount and emits Deposit(..., netAmount, ..., fee).

safeTransferFrom only requires the token not to revert / return false. A token that transfers netAmount - tax and returns true is treated as a full lock.

Fee and net are two transfers. A tax on each transfer can short both the fee recipient and the vault. Checking only the vault still leaves fee accounting wrong; the dest-credit hole is the vault short.

EVM: unlock and mint/burn already fail closed; lock deposit does not

LockUnlock.unlock:

initialThis / initialTo
safeTransfer(to, amount)
require finalThis == initialThis - amount  // InvalidUnlockThis
require finalTo   == initialTo + amount    // InvalidUnlockTo

MintBurn.burn / mint compare balanceOf around burnFrom / mint.

Lock inventory is not created by LockUnlock.lock (that path was removed). Bridge pushes tokens in. The vault never sees a lock-time delta, so unlock’s exact-delta check cannot protect deposit accounting. Unlock of a tax token typically fails on the recipient delta (InvalidUnlockTo) even when the vault debit matches amount (tax burned from sender). That does not fix dest mint of a standard wrapped identifier against a short lock.

depositERC20Mintable burns via MintBurn.burn (delta on the user). The fee safeTransferFrom on that path still has no recipient delta. Bundle a fee-recipient check there too; do not treat mintable as a separate issue.

Registration does not probe

TokenRegistry.registerToken(token, tokenType) (onlyOwner): sets tokenRegistered, tokenTypes, default rate limits from totalSupply(). No transfer / transferFrom probe. setTokenType likewise.

MockTransferTaxToken.sol implements 10% tax on transfer / transferFrom (recipient gets 90%; remainder burned from sender). No test file imports it. CODE_REVIEW.md still lists it as covering deflationary handling and leaves “integration test for fee-on-transfer tokens through full bridge cycle” unchecked.

Why the new implementation is needed

Docs and vault natspec claim FoT is unsupported, but the only lock-path transfer is on Bridge, which does not measure received amounts. Unlock/mint/burn checks are the wrong layer for deposit credits.
Crediting netAmount into the cross-chain hash while holding less is dest over-mint / inventory insolvency, not a UX warning.
Owner-only registration is not a control if listing is mistaken or a listed token later enables a fee. Fail closed on deposit and at register.
The FoT mock already exists and is unwired. CI cannot regress this until a deposit-of-tax-token fixture reverts.

Constraints / guardrails

Reject, do not haircut. If received ≠ netAmount (lock) or received ≠ fee (fee transfer), revert. Do not store the actual received amount. Hash / DepositRecord.amount / dest credit must stay requested-net for standard tokens only.
Do not weaken GuardBridge, rate limits, cancelers, (srcChain, nonce) replay, pause, nonReentrant, or fee forwarding to feeRecipient.
Do not pull tokens out of LockUnlock except via unlock. Do not add a recoverAsset backdoor for the shortfall.
depositNative is ETH msg.value (already exact). Out of scope except: do not “fix” FoT by wrapping native.
UUPS / storage: deposit checks are local variables; no new Bridge storage required. Registration probe must not blow TokenRegistry __gap (currently [38]). If adding a probe helper contract, it is a new deploy + owner wiring, not a packed overwrite of existing slots.
Registration probe must not be callable by non-owners as a token-drain. Owner-funded 1-unit round-trip (approve registry, transferFrom owner → registry, require delta == 1, return token to owner) is acceptable. Dust amount == 0 probes are not sufficient (many FoT tokens skip tax at 0).
Do not require a mainnet token upgrade. Do not change dest-token mapping or hash input order.
AccessManager / role-id remap stays out (#98). Native recover liability stays #179.
No community autoland. Do not add ready. No public mainnet FoT recipe.

Relevant files

Path	Why
`packages/contracts-evm/src/Bridge.sol`	`depositERC20` (and mintable fee transfer) records requested amounts with no received delta
`packages/contracts-evm/src/interfaces/IBridge.sol`	New revert for non-standard received amount (e.g. `NonStandardTokenTransfer`)
`packages/contracts-evm/src/LockUnlock.sol`	Unlock already exact-delta; lock is push-in from Bridge (comment at lock section)
`packages/contracts-evm/src/MintBurn.sol`	Burn/mint already exact-delta; mintable fee transfer is still on Bridge
`packages/contracts-evm/src/TokenRegistry.sol`	`registerToken` / `setTokenType` have no standard-transfer probe
`packages/contracts-evm/src/interfaces/ITokenRegistry.sol`	New registration revert (e.g. `NonStandardToken`)
`packages/contracts-evm/test/mocks/MockTransferTaxToken.sol`	Existing 10% FoT mock; unused
`packages/contracts-evm/test/Bridge.t.sol`	Deposit tests use standard mocks only
`packages/contracts-evm/test/TokenRegistry.t.sol`	Registration tests use dummy addresses; no transfer probe
`packages/contracts-evm/test/LockUnlock.t.sol`	Unlock delta tests; no Bridge deposit of FoT
`packages/contracts-evm/OPERATIONAL_NOTES.md`	§4 documents FoT as unsupported; not enforced on deposit
`packages/contracts-evm/CODE_REVIEW.md`	Claims mock tests FoT; integration checkbox still open

Recommended direction

Deposit fail-closed (required): Around each ERC-20 safeTransferFrom in depositERC20 (fee recipient and LockUnlock) and the fee transfer in depositERC20Mintable, snapshot balanceOf(to) before/after. Revert unless after - before == expected (and expected > 0 when that leg runs). Dedicated error. Do not update netAmount to the delta.
Registration fail-closed (required): registerToken (LockUnlock and MintBurn) runs an owner-funded 1-unit (or 1 smallest unit) round-trip probe: received must equal sent. Revert NonStandardToken (or equivalent) on mismatch, missing code, or failed return. Document that the owner must hold and approve that unit. setTokenType does not need a second probe if the token is already registered, but do not skip the probe on first register for MintBurn.
Tests: Import MockTransferTaxToken. Register + depositERC20 must revert (vault and, with feeBps > 0, fee recipient). Standard mock deposits still succeed with LockUnlock balance += netAmount. Unlock of FoT still reverts (existing unlock invariant). Registration of the tax mock reverts before it can be deposited.
Docs: §4 becomes an enforced invariant, not a recommendation. Strike CODE_REVIEW language that the mock already “tests deflationary token handling.”

Do not implement a “credit actual received” mode in this ticket. That is a different hash/accounting design.

Acceptance criteria

AC1. depositERC20 of a standard ERC-20: LockUnlock.balanceOf(token) increases by netAmount; fee recipient increases by fee; deposit record / hash still use netAmount.
AC2. depositERC20 of MockTransferTaxToken (registered only if the probe is bypassed in a unit test, or via a test-only hook not shipped to production): the lock safeTransferFrom reverts; no Deposit event; no deposit record; depositNonce unchanged.
AC3. With fee > 0, a token that delivers less than fee to feeRecipient reverts before the lock transfer; no partial lock.
AC4. registerToken of MockTransferTaxToken reverts after the 1-unit probe. Standard ERC-20 still registers. Non-owner still cannot register.
AC5. LockUnlock.unlock exact deltas unchanged. MintBurn burn/mint deltas unchanged. depositERC20Mintable fee transfer gains the same received check as lock fees.
AC6. Docs §4 and CODE_REVIEW match shipped behavior. No natspec that safeTransferFrom of netAmount is sufficient for FoT.
AC7. No ready label. No change to dest mapping or hash field order.

Test plan (functional paths)

#	Path	Expect
T1	Standard ERC-20 `depositERC20`, fee 0	LockUnlock += amount; record amount = amount
T2	Standard ERC-20 `depositERC20`, fee > 0	Fee recipient += fee; LockUnlock += net; record = net
T3	`MockTransferTaxToken` `depositERC20` (if registered via test harness)	Revert on received ≠ net (or ≠ fee first)
T4	`registerToken` tax mock with owner-funded 1 unit	Revert `NonStandardToken` (or equivalent)
T5	`registerToken` standard mock with 1-unit probe	Succeeds; token returned to owner
T6	`registerToken` still `onlyOwner`	Existing owner revert
T7	`depositERC20Mintable` standard + fee > 0	Fee recipient += fee; burn delta still exact
T8	`depositERC20Mintable` fee transfer shortfall	Revert; no burn
T9	Existing `depositERC20` dest/chain/zero-amount reverts	Unchanged
T10	`LockUnlock.unlock` standard token	Unchanged exact-delta success
T11	Probe `amount == 0` must not count as “standard”	Registration still requires a positive unit

Test plan (attack, hack, and abuse)

Non-exploitative. Local Forge only. Do not use these as a mainnet recipe.

#	Vector	Expect
A1	FoT lock deposit then dest mint of recorded net	Deposit reverts; no dest-credit hash
A2	FoT tax only on `transferFrom` to vault, fee 0	Revert; nonce unused
A3	Token returns `true` but recipient delta 0	Revert
A4	Rebasing token that changes `balanceOf` mid-tx (if a mock exists)	Deposit/register revert; do not add a live rebase exploit
A5	Probe that uses `amount == 0` to skip tax	Must not pass registration
A6	Non-owner calling the probe / `registerToken`	Owner revert; no token pull from third parties
A7	Partial success: fee transfer ok, lock short	Whole tx reverts (`nonReentrant`); no fee kept without lock (atomic)

Verification criteria

Forge: new tests in Bridge.t.sol and TokenRegistry.t.sol using MockTransferTaxToken. forge test in packages/contracts-evm green.
Grep: depositERC20 lock safeTransferFrom is preceded/followed by balanceOf snapshots and an equality check. MockTransferTaxToken is imported from a test file.
registerToken path contains a positive-amount transfer probe (or clearly named helper) that reverts on delta mismatch.
Docs: §4 “enforced on deposit and registration,” not “if in doubt, test with a mock.”
Do not verify by depositing a tax token on a production RPC.

Out of scope

Native ETH custody / recoverAsset (#179).
Guard/rate-limit fail-closed (#178), cancel-window timelock (#177), M-of-N approve (#176).
Supporting FoT by hashing actual received (product reject, not haircut).
Terra CW20 tax / Solana Token-2022 transfer-fee extensions (sibling patterns; do not silently retarget this EVM ticket). Token-2022 QA remains #97.
Live owner key / production token listing (ops).
AccessManager role-id remap (#98).

First-pass model recommendation

Recommendation: grok-high

Rationale: Security class plus founder-required contracts (Bridge.sol deposit accounting, TokenRegistry registration probe, LockUnlock / MintBurn invariant alignment). Composer is disallowed (High/security; contracts / wallet / 2-of-3). Deposit fail-closed vs “credit actual received” is a protocol hash/custody choice; a wrong delta (checking the user instead of LockUnlock, or probing amount == 0) leaves dest over-mint intact. Verify with Forge FoT-mock deposit/register fixtures, not a production deposit.

## Summary EVM `Bridge.depositERC20` computes `netAmount = amount - fee`, optionally `safeTransferFrom`s the fee to `feeConfig.feeRecipient`, then `safeTransferFrom`s `netAmount` to `LockUnlock`. Neither transfer measures `balanceOf` before/after. The deposit record, `Deposit` event, and `xchainHashId` all use the **requested** `netAmount`, not tokens actually received. `LockUnlock.unlock` already enforces exact deltas (`InvalidUnlockThis` / `InvalidUnlockTo`). `MintBurn.burn` / `mint` do the same. **Lock deposits do not.** Docs already list fee-on-transfer as unsupported (`OPERATIONAL_NOTES.md` §4). `TokenRegistry.registerToken` is owner-only and does **not** probe transfer semantics. This is **not** [#179](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/179) (native `recoverAsset` vs `depositNative` liability). This is **not** [#178](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/178) (unset `guardBridge` / `rateLimitBridge`). This is **not** [#100](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/100) (protocol fee vs rate-limit minimums). This is **not** [#9](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/9) (frontend displayed amount). Keyword overlap on `fee` / `depositERC20` / `netAmount` is not this bug. Internal review id: **EVM-M1**. Still in source as of 2026-09-12. Bundle (same ticket, do not split): 1. Measure lock-side (and fee) ERC-20 balance deltas on `depositERC20`; revert if received ≠ expected. Do **not** credit a reduced amount (that would change hash inputs). 2. Fail closed at `TokenRegistry.registerToken` for non-standard transfer semantics (owner-funded 1-unit round-trip probe, or equivalent). 3. Wire existing `MockTransferTaxToken` (10% tax, currently **unreferenced** by any test) through deposit + registration + unlock fixtures. Founder-required contracts. No community autoland. Do not add `ready`. ## Impact (today vs hypothetical) **Inventory / dest-credit invariant is broken in source today** whenever a fee-on-transfer (or otherwise deflationary-on-transfer) ERC-20 is registered as `LockUnlock`. `depositERC20` will record and hash `netAmount` while `LockUnlock` holds less. Destination mint/unlock of the recorded amount then over-credits relative to source inventory (mint path) or cannot be backed 1:1. Other lockers of the same token are short. Registration is `onlyOwner`, and §4 already says FoT is unsupported. That is an operational hope, not an invariant. Listing any tax/deflationary token (mistaken economic-token add, or a token that later turns on a fee) activates the hole with no on-chain reject. Hypothetical-only if every registered lock token is guaranteed standard `transfer`/`transferFrom` forever. Source still allows registering and depositing non-standard tokens. Do not publish a mainnet FoT deposit sequence, token-tax configuration, or live RPC probe scripts. ### EVM: `depositERC20` records requested net, not received `depositERC20` (`whenNotPaused`, `nonReentrant`): - Requires registered token, dest chain, `destToken != 0`, `amount > 0`. - `fee = calculateFee(msg.sender, amount)`; `netAmount = amount - fee`. - If `fee > 0`: `safeTransferFrom(msg.sender, feeRecipient, fee)` with **no** `balanceOf(feeRecipient)` delta check. - Then `safeTransferFrom(msg.sender, address(lockUnlock), netAmount)` with **no** `balanceOf(LockUnlock)` delta check. - Stores `deposits[xchainHashId].amount = netAmount` and emits `Deposit(..., netAmount, ..., fee)`. `safeTransferFrom` only requires the token not to revert / return false. A token that transfers `netAmount - tax` and returns `true` is treated as a full lock. Fee and net are **two** transfers. A tax on each transfer can short both the fee recipient and the vault. Checking only the vault still leaves fee accounting wrong; the dest-credit hole is the vault short. ### EVM: unlock and mint/burn already fail closed; lock deposit does not `LockUnlock.unlock`: ```text initialThis / initialTo safeTransfer(to, amount) require finalThis == initialThis - amount // InvalidUnlockThis require finalTo == initialTo + amount // InvalidUnlockTo ``` `MintBurn.burn` / `mint` compare `balanceOf` around `burnFrom` / `mint`. Lock inventory is **not** created by `LockUnlock.lock` (that path was removed). Bridge pushes tokens in. The vault never sees a lock-time delta, so unlock’s exact-delta check cannot protect deposit accounting. Unlock of a tax token typically fails on the **recipient** delta (`InvalidUnlockTo`) even when the vault debit matches `amount` (tax burned from sender). That does not fix dest **mint** of a standard wrapped identifier against a short lock. `depositERC20Mintable` burns via `MintBurn.burn` (delta on the user). The **fee** `safeTransferFrom` on that path still has no recipient delta. Bundle a fee-recipient check there too; do not treat mintable as a separate issue. ### Registration does not probe `TokenRegistry.registerToken(token, tokenType)` (`onlyOwner`): sets `tokenRegistered`, `tokenTypes`, default rate limits from `totalSupply()`. No `transfer` / `transferFrom` probe. `setTokenType` likewise. `MockTransferTaxToken.sol` implements 10% tax on `transfer` / `transferFrom` (recipient gets 90%; remainder burned from sender). **No test file imports it.** `CODE_REVIEW.md` still lists it as covering deflationary handling and leaves “integration test for fee-on-transfer tokens through full bridge cycle” unchecked. ## Why the new implementation is needed 1. Docs and vault natspec claim FoT is unsupported, but the only lock-path transfer is on `Bridge`, which does not measure received amounts. Unlock/mint/burn checks are the wrong layer for deposit credits. 2. Crediting `netAmount` into the cross-chain hash while holding less is dest over-mint / inventory insolvency, not a UX warning. 3. Owner-only registration is not a control if listing is mistaken or a listed token later enables a fee. Fail closed on deposit **and** at register. 4. The FoT mock already exists and is unwired. CI cannot regress this until a deposit-of-tax-token fixture reverts. ## Constraints / guardrails - **Reject, do not haircut.** If received ≠ `netAmount` (lock) or received ≠ `fee` (fee transfer), revert. Do not store the actual received amount. Hash / `DepositRecord.amount` / dest credit must stay requested-net for standard tokens only. - Do not weaken `GuardBridge`, rate limits, cancelers, `(srcChain, nonce)` replay, pause, `nonReentrant`, or fee forwarding to `feeRecipient`. - Do not pull tokens out of `LockUnlock` except via `unlock`. Do not add a `recoverAsset` backdoor for the shortfall. - `depositNative` is ETH `msg.value` (already exact). Out of scope except: do not “fix” FoT by wrapping native. - UUPS / storage: deposit checks are local variables; no new Bridge storage required. Registration probe must not blow `TokenRegistry` `__gap` (currently `[38]`). If adding a probe helper contract, it is a new deploy + owner wiring, not a packed overwrite of existing slots. - Registration probe must not be callable by non-owners as a token-drain. Owner-funded 1-unit round-trip (approve registry, `transferFrom` owner → registry, require delta == 1, return token to owner) is acceptable. Dust `amount == 0` probes are **not** sufficient (many FoT tokens skip tax at 0). - Do not require a mainnet token upgrade. Do not change dest-token mapping or hash input order. - AccessManager / role-id remap stays out (#98). Native recover liability stays #179. - No community autoland. Do not add `ready`. No public mainnet FoT recipe. ## Relevant files | Path | Why | | --- | --- | | `packages/contracts-evm/src/Bridge.sol` | `depositERC20` (and mintable fee transfer) records requested amounts with no received delta | | `packages/contracts-evm/src/interfaces/IBridge.sol` | New revert for non-standard received amount (e.g. `NonStandardTokenTransfer`) | | `packages/contracts-evm/src/LockUnlock.sol` | Unlock already exact-delta; lock is push-in from Bridge (comment at lock section) | | `packages/contracts-evm/src/MintBurn.sol` | Burn/mint already exact-delta; mintable **fee** transfer is still on Bridge | | `packages/contracts-evm/src/TokenRegistry.sol` | `registerToken` / `setTokenType` have no standard-transfer probe | | `packages/contracts-evm/src/interfaces/ITokenRegistry.sol` | New registration revert (e.g. `NonStandardToken`) | | `packages/contracts-evm/test/mocks/MockTransferTaxToken.sol` | Existing 10% FoT mock; unused | | `packages/contracts-evm/test/Bridge.t.sol` | Deposit tests use standard mocks only | | `packages/contracts-evm/test/TokenRegistry.t.sol` | Registration tests use dummy addresses; no transfer probe | | `packages/contracts-evm/test/LockUnlock.t.sol` | Unlock delta tests; no Bridge deposit of FoT | | `packages/contracts-evm/OPERATIONAL_NOTES.md` | §4 documents FoT as unsupported; not enforced on deposit | | `packages/contracts-evm/CODE_REVIEW.md` | Claims mock tests FoT; integration checkbox still open | ## Recommended direction 1. **Deposit fail-closed (required):** Around each ERC-20 `safeTransferFrom` in `depositERC20` (fee recipient and `LockUnlock`) and the fee transfer in `depositERC20Mintable`, snapshot `balanceOf(to)` before/after. Revert unless `after - before == expected` (and `expected > 0` when that leg runs). Dedicated error. Do not update `netAmount` to the delta. 2. **Registration fail-closed (required):** `registerToken` (LockUnlock and MintBurn) runs an owner-funded 1-unit (or 1 smallest unit) round-trip probe: received must equal sent. Revert `NonStandardToken` (or equivalent) on mismatch, missing code, or failed return. Document that the owner must hold and approve that unit. `setTokenType` does not need a second probe if the token is already registered, but **do not** skip the probe on first register for MintBurn. 3. **Tests:** Import `MockTransferTaxToken`. Register + `depositERC20` must revert (vault and, with `feeBps > 0`, fee recipient). Standard mock deposits still succeed with `LockUnlock` balance += `netAmount`. Unlock of FoT still reverts (existing unlock invariant). Registration of the tax mock reverts before it can be deposited. 4. **Docs:** §4 becomes an **enforced** invariant, not a recommendation. Strike CODE_REVIEW language that the mock already “tests deflationary token handling.” Do not implement a “credit actual received” mode in this ticket. That is a different hash/accounting design. ## Acceptance criteria - AC1. `depositERC20` of a standard ERC-20: `LockUnlock.balanceOf(token)` increases by `netAmount`; fee recipient increases by `fee`; deposit record / hash still use `netAmount`. - AC2. `depositERC20` of `MockTransferTaxToken` (registered only if the probe is bypassed in a unit test, or via a test-only hook **not** shipped to production): the lock `safeTransferFrom` **reverts**; no `Deposit` event; no deposit record; `depositNonce` unchanged. - AC3. With `fee > 0`, a token that delivers less than `fee` to `feeRecipient` reverts **before** the lock transfer; no partial lock. - AC4. `registerToken` of `MockTransferTaxToken` reverts after the 1-unit probe. Standard ERC-20 still registers. Non-owner still cannot register. - AC5. `LockUnlock.unlock` exact deltas unchanged. `MintBurn` burn/mint deltas unchanged. `depositERC20Mintable` fee transfer gains the same received check as lock fees. - AC6. Docs §4 and CODE_REVIEW match shipped behavior. No natspec that `safeTransferFrom` of `netAmount` is sufficient for FoT. - AC7. No `ready` label. No change to dest mapping or hash field order. ## Test plan (functional paths) | # | Path | Expect | | --- | --- | --- | | T1 | Standard ERC-20 `depositERC20`, fee 0 | LockUnlock += amount; record amount = amount | | T2 | Standard ERC-20 `depositERC20`, fee > 0 | Fee recipient += fee; LockUnlock += net; record = net | | T3 | `MockTransferTaxToken` `depositERC20` (if registered via test harness) | Revert on received ≠ net (or ≠ fee first) | | T4 | `registerToken` tax mock with owner-funded 1 unit | Revert `NonStandardToken` (or equivalent) | | T5 | `registerToken` standard mock with 1-unit probe | Succeeds; token returned to owner | | T6 | `registerToken` still `onlyOwner` | Existing owner revert | | T7 | `depositERC20Mintable` standard + fee > 0 | Fee recipient += fee; burn delta still exact | | T8 | `depositERC20Mintable` fee transfer shortfall | Revert; no burn | | T9 | Existing `depositERC20` dest/chain/zero-amount reverts | Unchanged | | T10 | `LockUnlock.unlock` standard token | Unchanged exact-delta success | | T11 | Probe `amount == 0` must not count as “standard” | Registration still requires a positive unit | ## Test plan (attack, hack, and abuse) Non-exploitative. Local Forge only. Do not use these as a mainnet recipe. | # | Vector | Expect | | --- | --- | --- | | A1 | FoT lock deposit then dest mint of recorded net | Deposit reverts; no dest-credit hash | | A2 | FoT tax only on `transferFrom` to vault, fee 0 | Revert; nonce unused | | A3 | Token returns `true` but recipient delta 0 | Revert | | A4 | Rebasing token that changes `balanceOf` mid-tx (if a mock exists) | Deposit/register revert; do not add a live rebase exploit | | A5 | Probe that uses `amount == 0` to skip tax | Must not pass registration | | A6 | Non-owner calling the probe / `registerToken` | Owner revert; no token pull from third parties | | A7 | Partial success: fee transfer ok, lock short | Whole tx reverts (`nonReentrant`); no fee kept without lock (atomic) | ## Verification criteria - Forge: new tests in `Bridge.t.sol` and `TokenRegistry.t.sol` using `MockTransferTaxToken`. `forge test` in `packages/contracts-evm` green. - Grep: `depositERC20` lock `safeTransferFrom` is preceded/followed by `balanceOf` snapshots and an equality check. `MockTransferTaxToken` is imported from a test file. - `registerToken` path contains a positive-amount transfer probe (or clearly named helper) that reverts on delta mismatch. - Docs: §4 “enforced on deposit and registration,” not “if in doubt, test with a mock.” - Do not verify by depositing a tax token on a production RPC. ## Out of scope - Native ETH custody / `recoverAsset` (#179). - Guard/rate-limit fail-closed (#178), cancel-window timelock (#177), M-of-N approve (#176). - Supporting FoT by hashing actual received (product reject, not haircut). - Terra CW20 tax / Solana Token-2022 transfer-fee extensions (sibling patterns; do not silently retarget this EVM ticket). Token-2022 QA remains #97. - Live owner key / production token listing (ops). - AccessManager role-id remap (#98). ## First-pass model recommendation Recommendation: grok-high Rationale: Security class plus founder-required contracts (`Bridge.sol` deposit accounting, `TokenRegistry` registration probe, `LockUnlock` / `MintBurn` invariant alignment). Composer is disallowed (High/security; contracts / wallet / 2-of-3). Deposit fail-closed vs “credit actual received” is a protocol hash/custody choice; a wrong delta (checking the user instead of `LockUnlock`, or probing `amount == 0`) leaves dest over-mint intact. Verify with Forge FoT-mock deposit/register fixtures, not a production deposit.

PlasticDigits added the

high-risk

security

labels

2026-09-12 13:07:47 +00:00

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

code/cl8y-bridge-monorepo#189

No description provided.

Rows
Columns