code/cl8y-bridge-monorepo

Fork 0

security(evm): stop recoverAsset from sweeping native ETH deposits #179

New issue

Open

opened 2026-09-12 12:24:14 +00:00 by PlasticDigits · 0 comments

PlasticDigits commented

2026-09-12 12:24:14 +00:00

Owner

Summary

EVM Bridge.depositNative keeps net native ETH on the Bridge proxy. ERC-20 lock deposits go to LockUnlock. recoverAsset(address(0), amount, recipient) is onlyOwner + whenPaused and sends any requested ETH amount with no native-liability check. Pause plus owner is enough to empty user native deposits.

This is not #178 (fail-closed when guardBridge / rateLimitBridge are unset). This is not #177 (cancel-window timelock / 15s floor). This is not #176 (on-chain M-of-N operator approve). This is not #175 (Terra WithdrawApprove ignoring min_signatures). This is not #135 (Terra wasm / config.admin handoff). Keyword overlap on depositNative / address(0) / “paused owner” is not this bug.

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

Bundle (same ticket, do not split):

Segregate fee ETH from bridged native ETH (fees already leave via feeRecipient; net deposit must not be an unbounded owner sweep).
Track native liabilities (sum of outstanding depositNative nets, minus any native-backed unlock path if one is added; accidental receive() / stray ETH is excess).
Restrict recoverAsset(address(0), …) to excess / stuck native only (address(this).balance - nativeLiability, or equivalent).
End-to-end Forge test: after a real depositNative, pause + recoverAsset(address(0), …) cannot take that user balance.

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

Impact (today vs hypothetical)

Funds at risk today in source. Native deposits are custodial ETH on the Bridge. The same owner() that can pause can, while paused, call recoverAsset(address(0), amount, anywhere) and take the entire native balance, including every unreclaimed depositNative net. Destination chains may already have minted/unlocked the wrapped equivalent against that hash. That is a custodial seize of bridged native, not a stuck-token sweeper.

This is not a public user-facing puzzle and does not need an exploit recipe. It requires the Bridge owner key (and pause). It is still a live privilege path against user native deposits. test_RecoverAsset_NativeETH currently encodes a full native drain (vm.deal + recover the full balance while paused).

Hypothetical-only if every production proxy has wrappedNative == address(0) (native deposits disabled) and no ETH is held. Source still allows native deposits and unbounded native recovery whenever wrappedNative is set.

Do not publish a mainnet pause-then-recover sequence or live RPC probe scripts.

EVM: `depositNative` retains raw ETH on Bridge

depositNative (whenNotPaused, nonReentrant):

Requires wrappedNative != 0, msg.value > 0, dest/chain/token mapping.
fee = calculateFee(msg.sender, msg.value); netAmount = msg.value - fee.
Fee (if any) is forwarded immediately to feeConfig.feeRecipient.
Net ETH stays on the Bridge. There is no wrap-to-WETH and no transfer into LockUnlock.
wrappedNative is only the cross-chain token identifier (OPERATIONAL_NOTES.md §6). Deposit record stores token: wrappedNative and amount: netAmount.

ERC-20 lock path (depositERC20) safeTransferFroms net tokens to LockUnlock. Native and ERC-20 lock custody are not the same vault.

receive() external payable {} accepts arbitrary ETH with no accounting. That balance is mixed with deposit nets.

CODE_REVIEW.md still claims depositNative wraps to WETH. That is stale; current Bridge.sol does not wrap.

EVM: `recoverAsset` has no native-liability cap

recoverAsset(token, amount, recipient)
  onlyOwner whenPaused nonReentrant
  recipient != 0
  if token == address(0): recipient.call{value: amount}("")
  else IERC20(token).safeTransfer(recipient, amount)
  emit AssetRecovered

No check that amount <= address(this).balance - nativeLiability. There is no nativeLiability (or equivalent) storage. For ERC-20, user lock inventory lives on LockUnlock, so Bridge-held ERC-20 is typically stray; native user inventory is the Bridge balance. The ERC-20 recovery shape is therefore not a safe template for token == address(0).

withdrawExecuteUnlock pays ERC-20 from LockUnlock, not ETH from Bridge. There is no native-ETH unlock that decrements a tracked liability. Destination mint/unlock of the wrapped identifier can complete while source ETH remains fully owner-recoverable.

Tests lock in the sweep

packages/contracts-evm/test/Bridge.t.sol:

test_RecoverAsset_NativeETH — vm.deal(bridge, 5 ether), pause, recover all 5 ETH to 0xBEEF.
test_RecoverAsset_RevertsIfNotPaused / RevertsIfNotOwner — access only; no liability invariant.
No test that depositNative then pause then recover of the net amount reverts.
No test that recovery of excess (forced ETH above liability) still works.

Why the new implementation is needed

Stuck-fund recovery was added as the remediations for “ETH stuck on upgrade.” It was implemented as an unbounded owner transfer of the whole native balance. That is a seize API, not a sweeper.
ERC-20 lock and native deposit do not share a vault. Copying ERC-20 recoverAsset onto address(0) ignores that difference.
Pause is the incident brake. Recovery that can empty user native while paused turns pause into a drain switch.
Tests currently prove the drain succeeds. CI will not catch a regression until there is a “cannot take depositNative net” fixture.

Constraints / guardrails

Do not remove pause, onlyOwner, or nonReentrant on recovery. Restrict amount, do not open recovery to non-owners or while unpaused.
Do not weaken GuardBridge, rate limits, cancelers, (srcChain, nonce) replay, or fee forwarding to feeRecipient.
Fees on depositNative already leave the contract. Do not double-count fee ETH as liability. Liability is outstanding net native deposits (and any other native the protocol still owes), not msg.value gross.
receive() stray ETH and explicit vm.deal / mis-sent ETH remain recoverable as excess.
UUPS / storage: if adding nativeLiability (or two buckets: liability vs excess), consume __gap correctly. Do not break existing proxy layout. Prefer a new storage field + gap shrink over a packed overwrite. New initializer version only if a field must be set at init (default zero is fine).
Wrapping native into WETH and locking WETH in LockUnlock is an acceptable alternative if it preserves current cross-chain identifier semantics (wrappedNative as dest token id), fee-ETH forwarding, and existing deposit hashes. Do not silently change dest-token mapping or hash inputs. If wrapping is chosen, recovery of WETH must still not pull locked inventory from LockUnlock except via the normal unlock path.
Do not implement wrapping and a parallel unbounded recoverAsset(address(0)) on leftover ETH. One custody model.
LockUnlock stays ERC-20. Do not invent native ETH unlock on LockUnlock unless tests and token-type routing are updated in this same change.
AccessManager / role-id remap stays out (#98). This ticket does not retarget #177 owner 2-step, but a later 2-step owner does not replace a liability cap.
No community autoland. Do not add ready. No public mainnet recover recipe.

Relevant files

Path	Why
`packages/contracts-evm/src/Bridge.sol`	`depositNative` retains ETH; `recoverAsset` unbounded native send; `receive()` mixes stray ETH; `__gap`
`packages/contracts-evm/src/interfaces/IBridge.sol`	`AssetRecovered`; no recovery-cap / liability surface on the interface today
`packages/contracts-evm/src/LockUnlock.sol`	ERC-20 lock vault; native is not here today
`packages/contracts-evm/test/Bridge.t.sol`	`test_RecoverAsset_NativeETH` encodes full drain; missing depositNative-vs-recover invariant
`packages/contracts-evm/OPERATIONAL_NOTES.md`	§6 documents raw-ETH retain; no recovery-cap rule
`packages/contracts-evm/CODE_REVIEW.md`	Stale “wraps ETH to WETH” claim
`docs/contracts-evm.md` / `docs/crosschain-flows.md`	Native deposit flow docs

Recommended direction

Liability accounting (preferred, minimal custody change): nativeLiability += netAmount in depositNative. Decrement only when a future native-backed source unlock exists (none today — so liability stays until a defined release, or until dest-side mint/unlock is explicitly not a source-ETH release). recoverAsset(address(0), amount, …) reverts unless amount <= address(this).balance - nativeLiability (and amount > 0). Dedicated error (e.g. RecoveryExceedsExcess).
Or wrap-and-lock: depositNative wraps net to WETH and LockUnlocks it like ERC-20. Bridge native balance then should be ~0 plus stray receive(). recoverAsset(address(0)) only sweeps excess ETH; WETH recovery must not exceed tokens sitting on Bridge (not in LockUnlock). Update hash/token-id tests so wrappedNative identity is unchanged.
Fee vs bridged: Keep fee call{value: fee} to feeRecipient before liability/wrap. Never recover fee ETH that already left. Do not route fees through LockUnlock.
Tests replace the drain fixture: test_RecoverAsset_NativeETH must not prove a full-balance sweep after a user deposit. Split: (a) stray/receive/deal excess is recoverable while paused; (b) depositNative net is not recoverable; (c) recovering nativeLiability + 1 reverts; (d) owner/pause checks remain.
Docs: OPERATIONAL_NOTES.md §6 + recovery: owner may recover excess/stuck native only. Strike wrap-to-WETH language in CODE_REVIEW.md unless wrapping actually ships.

Acceptance criteria

AC1. After depositNative of V with fee F, Bridge ETH increases by V - F (plus any prior balance). recoverAsset(address(0), net, recipient) while paused reverts. User native remains on the contract (or in LockUnlock as WETH if wrapping is chosen).
AC2. Excess native (ETH sent via receive(), or balance - nativeLiability) can be recovered while paused + owner, and only that excess.
AC3. recoverAsset still reverts if not paused or not owner. recipient == 0 still reverts.
AC4. ERC-20 recoverAsset still cannot pull tokens out of LockUnlock (only tokens actually on Bridge). Do not add a LockUnlock backdoor.
AC5. Fee recipient still receives fee ETH on depositNative; fees are not later swept as “bridged.”
AC6. Existing deposit hash / wrappedNative identifier / dest mapping behavior unchanged unless wrapping is the chosen custody model and tests are updated in this change.
AC7. Docs match the shipped custody model. No “recovery can send any ETH” natspec.

Test plan (functional paths)

#	Path	Expect
T1	`depositNative` value `V`, fee `F`	Bridge balance += `V-F`; fee recipient += `F`; deposit record amount `V-F`
T2	Pause, `recoverAsset(0, net, to)`	Revert (`RecoveryExceedsExcess` or equivalent); Bridge still holds net
T3	`receive()` (or deal) extra `X` after T1, recover `X`	Succeeds; user net still on Bridge / locked WETH
T4	Recover `X + 1` after T3	Revert
T5	Unpaused `recoverAsset`	Existing pause revert
T6	Non-owner after pause	Existing owner revert
T7	ERC-20 tokens minted onto Bridge (not LockUnlock), recover	Still works (stuck ERC-20)
T8	ERC-20 locked via `depositERC20` into LockUnlock, recover that amount from Bridge	Transfer fails or recovers 0 / does not debit LockUnlock
T9	`wrappedNative == 0`	`depositNative` still `WrappedNativeNotSet`; no new hole
T10	If wrapping ships: LockUnlock WETH balance += net; Bridge native += 0 after fee	Unlock path still ERC-20 WETH, not raw ETH, unless explicitly designed and tested

Test plan (attack, hack, and abuse)

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

#	Vector	Expect
A1	Pause then recover full `address(this).balance` after user `depositNative`	Revert; user net intact
A2	Recover to owner / arbitrary EOA	Still capped to excess only
A3	Multiple deposits, recover sum of nets	Revert
A4	Fee-on / fee-off (`feeBps` 0 vs nonzero)	Liability is net, not gross; fee already paid out is not “excess” on Bridge
A5	`receive()` mix-in then recover “all”	Only the mix-in (excess) leaves; nets stay
A6	Upgrade / reinitializer must not zero liability while ETH remains	Storage layout / gap test; liability survives proxy upgrade fixture if one exists
A7	ERC-20 recover used as a way to pull WETH from LockUnlock	Must not; only Bridge-held ERC-20

Verification criteria

Forge: replace/extend test_RecoverAsset_* with T1–T8 and A1–A5 in Bridge.t.sol. forge test in packages/contracts-evm green, including invariant tests if Bridge.inv.t.sol covers balances.
Grep: recoverAsset native branch no longer call{value: amount} without an excess/liability check.
If wrapping: depositNative contains wrap + transfer to LockUnlock; tests show WETH on LockUnlock, not raw ETH equal to net on Bridge.
Docs: §6 + recovery natspec match. CODE_REVIEW.md wrap claim either true or deleted.
Do not verify by pausing or recovering on a production RPC.

Out of scope

Terra RecoverAsset / LOCKED_BALANCES (admin recover of native denom without decrementing locked balances is a sibling pattern; do not silently retarget this EVM ticket into a CosmWasm rewrite). File or extend Terra separately if product wants parity.
Solana (no recoverAsset in program sources searched).
Guard/rate-limit fail-closed (#178), cancel-window timelock (#177), M-of-N approve (#176).
Live owner pause / key rotation / production ETH movement (ops, not this issue).
AccessManager role-id remap (#98).

First-pass model recommendation

Recommendation: grok-high

Rationale: Security class plus founder-required contracts (Bridge.sol custody + recoverAsset + possible LockUnlock / WETH wrap + storage gap). Composer is disallowed (High/security; contracts / wallet / 2-of-3; native-liability vs wrap is a protocol custody change, not a local three-file edit). A wrong cap (using balance without subtracting liability, or wrapping without moving ETH) leaves user native owner-sweepable. Verify with Forge deposit-then-paused-recover fixtures, not a production recover.

## Summary EVM `Bridge.depositNative` keeps net native ETH on the Bridge proxy. ERC-20 lock deposits go to `LockUnlock`. `recoverAsset(address(0), amount, recipient)` is `onlyOwner` + `whenPaused` and sends **any** requested ETH amount with **no** native-liability check. Pause plus owner is enough to empty user native deposits. This is **not** [#178](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/178) (fail-closed when `guardBridge` / `rateLimitBridge` are unset). This is **not** [#177](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/177) (cancel-window timelock / 15s floor). This is **not** [#176](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/176) (on-chain M-of-N operator approve). This is **not** [#175](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/175) (Terra `WithdrawApprove` ignoring `min_signatures`). This is **not** [#135](https://git.cl8y.com/code/cl8y-bridge-monorepo/issues/135) (Terra wasm / `config.admin` handoff). Keyword overlap on `depositNative` / `address(0)` / “paused owner” is not this bug. Internal review id: **EVM-H4**. Still in source as of 2026-09-12. Bundle (same ticket, do not split): 1. Segregate fee ETH from bridged native ETH (fees already leave via `feeRecipient`; net deposit must not be an unbounded owner sweep). 2. Track native liabilities (sum of outstanding `depositNative` nets, minus any native-backed unlock path if one is added; accidental `receive()` / stray ETH is excess). 3. Restrict `recoverAsset(address(0), …)` to **excess / stuck** native only (`address(this).balance - nativeLiability`, or equivalent). 4. End-to-end Forge test: after a real `depositNative`, pause + `recoverAsset(address(0), …)` **cannot** take that user balance. Founder-required contracts. No community autoland. Do not add `ready`. ## Impact (today vs hypothetical) **Funds at risk today in source.** Native deposits are custodial ETH on the Bridge. The same `owner()` that can pause can, while paused, call `recoverAsset(address(0), amount, anywhere)` and take the entire native balance, including every unreclaimed `depositNative` net. Destination chains may already have minted/unlocked the wrapped equivalent against that hash. That is a custodial seize of bridged native, not a stuck-token sweeper. This is not a public user-facing puzzle and does not need an exploit recipe. It requires the Bridge owner key (and pause). It is still a live privilege path against user native deposits. `test_RecoverAsset_NativeETH` currently **encodes** a full native drain (`vm.deal` + recover the full balance while paused). Hypothetical-only if every production proxy has `wrappedNative == address(0)` (native deposits disabled) **and** no ETH is held. Source still allows native deposits and unbounded native recovery whenever `wrappedNative` is set. Do not publish a mainnet pause-then-recover sequence or live RPC probe scripts. ### EVM: `depositNative` retains raw ETH on Bridge `depositNative` (`whenNotPaused`, `nonReentrant`): - Requires `wrappedNative != 0`, `msg.value > 0`, dest/chain/token mapping. - `fee = calculateFee(msg.sender, msg.value)`; `netAmount = msg.value - fee`. - Fee (if any) is forwarded immediately to `feeConfig.feeRecipient`. - Net ETH **stays on the Bridge**. There is no wrap-to-WETH and no transfer into `LockUnlock`. - `wrappedNative` is only the cross-chain token **identifier** (`OPERATIONAL_NOTES.md` §6). Deposit record stores `token: wrappedNative` and `amount: netAmount`. ERC-20 lock path (`depositERC20`) `safeTransferFrom`s net tokens **to `LockUnlock`**. Native and ERC-20 lock custody are not the same vault. `receive() external payable {}` accepts arbitrary ETH with no accounting. That balance is mixed with deposit nets. `CODE_REVIEW.md` still claims `depositNative` wraps to WETH. That is stale; current `Bridge.sol` does not wrap. ### EVM: `recoverAsset` has no native-liability cap ```text recoverAsset(token, amount, recipient) onlyOwner whenPaused nonReentrant recipient != 0 if token == address(0): recipient.call{value: amount}("") else IERC20(token).safeTransfer(recipient, amount) emit AssetRecovered ``` No check that `amount <= address(this).balance - nativeLiability`. There is no `nativeLiability` (or equivalent) storage. For ERC-20, user lock inventory lives on `LockUnlock`, so Bridge-held ERC-20 is typically stray; native user inventory **is** the Bridge balance. The ERC-20 recovery shape is therefore not a safe template for `token == address(0)`. `withdrawExecuteUnlock` pays ERC-20 from `LockUnlock`, not ETH from Bridge. There is no native-ETH unlock that decrements a tracked liability. Destination mint/unlock of the wrapped identifier can complete while source ETH remains fully owner-recoverable. ### Tests lock in the sweep `packages/contracts-evm/test/Bridge.t.sol`: - `test_RecoverAsset_NativeETH` — `vm.deal(bridge, 5 ether)`, pause, recover **all** 5 ETH to `0xBEEF`. - `test_RecoverAsset_RevertsIfNotPaused` / `RevertsIfNotOwner` — access only; no liability invariant. - No test that `depositNative` then pause then recover of the net amount reverts. - No test that recovery of **excess** (forced ETH above liability) still works. ## Why the new implementation is needed 1. Stuck-fund recovery was added as the remediations for “ETH stuck on upgrade.” It was implemented as an unbounded owner transfer of the whole native balance. That is a seize API, not a sweeper. 2. ERC-20 lock and native deposit do not share a vault. Copying ERC-20 `recoverAsset` onto `address(0)` ignores that difference. 3. Pause is the incident brake. Recovery that can empty user native while paused turns pause into a drain switch. 4. Tests currently prove the drain succeeds. CI will not catch a regression until there is a “cannot take `depositNative` net” fixture. ## Constraints / guardrails - Do not remove pause, `onlyOwner`, or `nonReentrant` on recovery. Restrict **amount**, do not open recovery to non-owners or while unpaused. - Do not weaken `GuardBridge`, rate limits, cancelers, `(srcChain, nonce)` replay, or fee forwarding to `feeRecipient`. - Fees on `depositNative` already leave the contract. Do not double-count fee ETH as liability. Liability is outstanding **net** native deposits (and any other native the protocol still owes), not `msg.value` gross. - `receive()` stray ETH and explicit `vm.deal` / mis-sent ETH remain recoverable as excess. - UUPS / storage: if adding `nativeLiability` (or two buckets: liability vs excess), consume `__gap` correctly. Do not break existing proxy layout. Prefer a new storage field + gap shrink over a packed overwrite. New initializer version only if a field must be set at init (default zero is fine). - Wrapping native into WETH and locking WETH in `LockUnlock` is an acceptable alternative **if** it preserves current cross-chain identifier semantics (`wrappedNative` as dest token id), fee-ETH forwarding, and existing deposit hashes. Do not silently change dest-token mapping or hash inputs. If wrapping is chosen, recovery of WETH must still not pull locked inventory from `LockUnlock` except via the normal unlock path. - Do not implement wrapping **and** a parallel unbounded `recoverAsset(address(0))` on leftover ETH. One custody model. - `LockUnlock` stays ERC-20. Do not invent native ETH unlock on `LockUnlock` unless tests and token-type routing are updated in this same change. - AccessManager / role-id remap stays out (#98). This ticket does not retarget #177 owner 2-step, but a later 2-step owner does **not** replace a liability cap. - No community autoland. Do not add `ready`. No public mainnet recover recipe. ## Relevant files | Path | Why | | --- | --- | | `packages/contracts-evm/src/Bridge.sol` | `depositNative` retains ETH; `recoverAsset` unbounded native send; `receive()` mixes stray ETH; `__gap` | | `packages/contracts-evm/src/interfaces/IBridge.sol` | `AssetRecovered`; no recovery-cap / liability surface on the interface today | | `packages/contracts-evm/src/LockUnlock.sol` | ERC-20 lock vault; native is **not** here today | | `packages/contracts-evm/test/Bridge.t.sol` | `test_RecoverAsset_NativeETH` encodes full drain; missing depositNative-vs-recover invariant | | `packages/contracts-evm/OPERATIONAL_NOTES.md` | §6 documents raw-ETH retain; no recovery-cap rule | | `packages/contracts-evm/CODE_REVIEW.md` | Stale “wraps ETH to WETH” claim | | `docs/contracts-evm.md` / `docs/crosschain-flows.md` | Native deposit flow docs | ## Recommended direction 1. **Liability accounting (preferred, minimal custody change):** `nativeLiability += netAmount` in `depositNative`. Decrement only when a future native-backed source unlock exists (none today — so liability stays until a defined release, or until dest-side mint/unlock is explicitly **not** a source-ETH release). `recoverAsset(address(0), amount, …)` reverts unless `amount <= address(this).balance - nativeLiability` (and `amount > 0`). Dedicated error (e.g. `RecoveryExceedsExcess`). 2. **Or wrap-and-lock:** `depositNative` wraps net to WETH and `LockUnlock`s it like ERC-20. Bridge native balance then should be ~0 plus stray `receive()`. `recoverAsset(address(0))` only sweeps excess ETH; WETH recovery must not exceed tokens sitting on Bridge (not in `LockUnlock`). Update hash/token-id tests so `wrappedNative` identity is unchanged. 3. **Fee vs bridged:** Keep fee `call{value: fee}` to `feeRecipient` before liability/wrap. Never recover fee ETH that already left. Do not route fees through `LockUnlock`. 4. **Tests replace the drain fixture:** `test_RecoverAsset_NativeETH` must not prove a full-balance sweep after a user deposit. Split: (a) stray/`receive`/deal excess is recoverable while paused; (b) `depositNative` net is **not** recoverable; (c) recovering `nativeLiability + 1` reverts; (d) owner/pause checks remain. 5. **Docs:** `OPERATIONAL_NOTES.md` §6 + recovery: owner may recover excess/stuck native only. Strike wrap-to-WETH language in `CODE_REVIEW.md` unless wrapping actually ships. ## Acceptance criteria - AC1. After `depositNative` of `V` with fee `F`, Bridge ETH increases by `V - F` (plus any prior balance). `recoverAsset(address(0), net, recipient)` while paused **reverts**. User native remains on the contract (or in `LockUnlock` as WETH if wrapping is chosen). - AC2. Excess native (ETH sent via `receive()`, or `balance - nativeLiability`) **can** be recovered while paused + owner, and only that excess. - AC3. `recoverAsset` still reverts if not paused or not owner. `recipient == 0` still reverts. - AC4. ERC-20 `recoverAsset` still cannot pull tokens out of `LockUnlock` (only tokens actually on Bridge). Do not add a LockUnlock backdoor. - AC5. Fee recipient still receives fee ETH on `depositNative`; fees are not later swept as “bridged.” - AC6. Existing deposit hash / `wrappedNative` identifier / dest mapping behavior unchanged unless wrapping is the chosen custody model and tests are updated in this change. - AC7. Docs match the shipped custody model. No “recovery can send any ETH” natspec. ## Test plan (functional paths) | # | Path | Expect | | --- | --- | --- | | T1 | `depositNative` value `V`, fee `F` | Bridge balance += `V-F`; fee recipient += `F`; deposit record amount `V-F` | | T2 | Pause, `recoverAsset(0, net, to)` | Revert (`RecoveryExceedsExcess` or equivalent); Bridge still holds net | | T3 | `receive()` (or deal) extra `X` after T1, recover `X` | Succeeds; user net still on Bridge / locked WETH | | T4 | Recover `X + 1` after T3 | Revert | | T5 | Unpaused `recoverAsset` | Existing pause revert | | T6 | Non-owner after pause | Existing owner revert | | T7 | ERC-20 tokens minted onto Bridge (not LockUnlock), recover | Still works (stuck ERC-20) | | T8 | ERC-20 locked via `depositERC20` into LockUnlock, recover that amount from Bridge | Transfer fails or recovers 0 / does not debit LockUnlock | | T9 | `wrappedNative == 0` | `depositNative` still `WrappedNativeNotSet`; no new hole | | T10 | If wrapping ships: LockUnlock WETH balance += net; Bridge native += 0 after fee | Unlock path still ERC-20 WETH, not raw ETH, unless explicitly designed and tested | ## Test plan (attack, hack, and abuse) Non-exploitative. Local Forge only. Do not use these as a mainnet recipe. | # | Vector | Expect | | --- | --- | --- | | A1 | Pause then recover full `address(this).balance` after user `depositNative` | Revert; user net intact | | A2 | Recover to owner / arbitrary EOA | Still capped to excess only | | A3 | Multiple deposits, recover sum of nets | Revert | | A4 | Fee-on / fee-off (`feeBps` 0 vs nonzero) | Liability is net, not gross; fee already paid out is not “excess” on Bridge | | A5 | `receive()` mix-in then recover “all” | Only the mix-in (excess) leaves; nets stay | | A6 | Upgrade / reinitializer must not zero liability while ETH remains | Storage layout / gap test; liability survives proxy upgrade fixture if one exists | | A7 | ERC-20 recover used as a way to pull WETH from LockUnlock | Must not; only Bridge-held ERC-20 | ## Verification criteria - Forge: replace/extend `test_RecoverAsset_*` with T1–T8 and A1–A5 in `Bridge.t.sol`. `forge test` in `packages/contracts-evm` green, including invariant tests if `Bridge.inv.t.sol` covers balances. - Grep: `recoverAsset` native branch no longer `call{value: amount}` without an excess/liability check. - If wrapping: `depositNative` contains wrap + transfer to `LockUnlock`; tests show WETH on `LockUnlock`, not raw ETH equal to net on Bridge. - Docs: §6 + recovery natspec match. `CODE_REVIEW.md` wrap claim either true or deleted. - Do not verify by pausing or recovering on a production RPC. ## Out of scope - Terra `RecoverAsset` / `LOCKED_BALANCES` (admin recover of native denom without decrementing locked balances is a **sibling** pattern; do not silently retarget this EVM ticket into a CosmWasm rewrite). File or extend Terra separately if product wants parity. - Solana (no `recoverAsset` in program sources searched). - Guard/rate-limit fail-closed (#178), cancel-window timelock (#177), M-of-N approve (#176). - Live owner pause / key rotation / production ETH movement (ops, not this issue). - AccessManager role-id remap (#98). ## First-pass model recommendation Recommendation: grok-high Rationale: Security class plus founder-required contracts (`Bridge.sol` custody + `recoverAsset` + possible `LockUnlock` / WETH wrap + storage gap). Composer is disallowed (High/security; contracts / wallet / 2-of-3; native-liability vs wrap is a protocol custody change, not a local three-file edit). A wrong cap (using `balance` without subtracting liability, or wrapping without moving ETH) leaves user native owner-sweepable. Verify with Forge deposit-then-paused-recover fixtures, not a production recover.

PlasticDigits added the

high-risk

security

labels

2026-09-12 12:24:17 +00:00

PlasticDigits referenced this issue

2026-09-12 13:03:16 +00:00

security(bridge): pause must stop withdraw approve or reset cancel window #188

PlasticDigits referenced this issue

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

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