chore(audit): round 2 red-team fixes + bindings v0.13.0#125
Merged
Conversation
Round 2 ran five adversarial red-team agents with explicit attacker mindsets (economic / MEV searcher, operator collusion, governance takeover, cross-chain bridge, storage / upgrade / init) and Slither static analysis. ~50 new findings on top of Round 1; this commit lands the small-but-critical fixes that don't require interface-level redesign. CRITICAL — beacon SSZ endianness (B-01) - `BeaconChainProofs.getEffectiveBalanceGwei`, `getActivationEpoch`, `getExitEpoch`, `getWithdrawableEpoch`, and `_extractBalanceFromLeaf` now perform the correct little-endian byte-swap on SSZ-packed uint64 fields. The consensus layer packs values into the HIGH 8 bytes of a bytes32 chunk in little-endian; the previous code read the LOW 64 bits as if big-endian, returning 0 (or a byte-swapped wrong value) for every effective balance, exit epoch, and validator balance proven from a real EigenPod-CLI fixture. In-tree tests passed only because the fixtures mirrored the same wrong packing. Fixtures are now SSZ-correct and `test_extractBalance_RealSszLeaf_32ETH` pins the canonical 32-ETH encoding. CRITICAL — TNTLockFactory delegate-on-init airdrop capture (gov #1) - `getOrCreateLock` requires `msg.sender == beneficiary`. Without the gate, anyone could front-run the victim's first interaction with the deterministic lock, supply themselves as `delegatee`, and persistently capture every future inbound TNT transfer's voting power. HIGH — JobQuoteDetails missing requester (Round 2 economic F1, BREAKING) - `Types.JobQuoteDetails` now carries `address requester` and the `JOB_QUOTE_TYPEHASH` includes it. Any `_permittedCaller` (or anyone watching the mempool) could previously lift another caller's signed per-job RFQ quote and consume it for themselves. Off-chain signers MUST update. HIGH — wildcard quote rejection - `verifyQuoteBatch` rejects `requester == address(0)` outright. Wildcard quotes have no good production use case and let mempool observers race the legitimate caller for the single-use digest. HIGH — slash dispute dead-zone (Round 2 economic F4) - `SlashingLib.disputeSlash` window extends through `executeAfter + TIMESTAMP_BUFFER`, mirroring `isExecutable`. The prior asymmetry created a deterministic 15-second window where a sequencer could land an operator's dispute (revert) and then 15s later anyone could call `executeSlash`. Operator dispute and execute now use the same buffer. HIGH — SLASH_ADMIN self-dispute (Round 2 governance #4) - A SLASH_ADMIN that is also the proposer can no longer self-dispute. Without this, a single role-holder could propose, immediately self-dispute (no bond), and freeze operator stake for the full `disputeResolutionDeadline` window — and (when treasury == admin) capture the operator's bond on auto-execution. HIGH — L2 slash CEI (Round 1 deferred S-1) - `L2SlashingReceiver._handleSlashMessage` now applies the slash BEFORE consuming the nonce. If `canSlash` returns false (paused, unknown operator, etc.) or `slashBps == 0`, the call reverts so the bridge keeps the message available for retry. Previously the nonce was consumed first and the slash silently dropped on transient failure, locking that slash out forever. HIGH — L2 setMessenger / setSlasher timelock (Round 2 cross-chain C-2) - Both swaps now require `SENDER_ACTIVATION_DELAY` (2 days) for non-bootstrap changes. A compromised owner could otherwise hot-swap the messenger to a contract they control and impersonate any previously-authorised sender, undercutting the H-4 timelock on `authorizedSenders`. The first swap (when current is unset) is a bootstrap exemption so deploy scripts can wire the bridge without deadlocking. MEDIUM — MBSM grace-period pinning (Round 1 deferred gov H-3) - `MBSMRegistry.pinBlueprint` rejects revisions already scheduled for deprecation. Pinning during the grace window meant `getMBSM` returned `address(0)` the moment `completeDeprecation` ran, breaking every BSM call for the pinned blueprint. MEDIUM — forceRemoveOperator min-operators floor (Round 2 collusion #7) - A blueprint manager can no longer evict operators below `minOperators` unless their BSM explicitly opts in via the new `forceRemoveAllowsBelowMin(serviceId)` hook. The previous unconditional bypass let a malicious BSM bias the operator set toward sybils. Default implementation in `BlueprintServiceManagerBase` returns `false`, enforcing the floor. MEDIUM — slash `proposeSlash` rejects `bytes32(0)` evidence (Round 1) - Off-chain monitors keying off non-zero evidence no longer see silently-zero entries. LOW — `Types.ServiceRequest.activated` moved to end of struct - Upgrade-safety: appending the field at the end means a hypothetical upgrade from a pre-`activated` storage layout cannot accidentally read a non-zero byte from a different field as `activated == true`. Test suite - `test/security/AuditFixesTest.t.sol` adds `disputeSlash` admin self- dispute regression test on top of the Round 1 entries. - New `test_extractBalance_RealSszLeaf_32ETH` pins SSZ uint64 decoding. - `BeaconTestBase._generateValidatorFields` and `_generateBalanceRoot` now use SSZ-correct packing via the `_sszUint64` / `_reverseUint64` helpers. - Updated quote-typehash literals + signing helpers across every test file that constructs `QuoteDetails` / `JobQuoteDetails`. - EIP712 cross-repo deterministic vectors regenerated for the new typehash. Rust `pricing-engine/tests/eip712_compat.rs` MUST be regenerated to match. Bindings - Regenerated via `cargo xtask gen-bindings`. Bumped to 0.13.0 (BREAKING: typehash changes; consumers MUST update their EIP-712 payloads). - Slither sweep: 1279 results, mostly unused-state-variable + cache-array-length + constable-states (all informational on upgradeable contracts). Two real findings flagged for follow-up: `ServiceFeeDistributor._claimAllForToken` reentrancy-eth pattern (state-after-external-call inside a nonReentrant-guarded entry, needs CEI review) and `RebasingAssetAdapter` first-depositor inflation surface (needs virtual-offset migration; deferred). Findings deferred for follow-up PRs (out of scope here) - Cross-chain C-3: receiver-rotation replay across re-deployments. - Cross-chain H-1: Arbitrum L2 fee leak to inaccessible alias. - Cross-chain H-5: BSM `onSlash` bypass on beacon-slash path. - Cross-chain H-6: multi-pod operator under-slash by ~9-15%. - Operator collusion 2c: bitmap snapshot binding for aggregated jobs (swap-and-pop reorder breaks in-flight signatures). - Economic F2: `RebasingAssetAdapter` virtual-offset migration. - Economic F3: `cancelSlash` bond-refund reentrancy via disputer. - Economic F5: stake-just-in-time `billSubscription` arbitrage. - Economic F6: fee-on-transfer / rebasing token escrow drift. - Storage F-3: missing `__gap` on five rewards/* UUPS implementations. - Governance #5: ERC20Burnable burn-before-snapshot quorum suppression. - Governance #8: 50-action proposals + low threshold = privileged-call obfuscation surface. - `ServiceFeeDistributor._claimAllForToken` reentrancy review. The two governance upgrade tests pre-existing on origin/main (`test_GovernorSelfUpgrade`, `test_TokenUpgradeViaGovernance`) remain broken; not introduced by this PR.
This was referenced May 8, 2026
drewstone
added a commit
that referenced
this pull request
May 8, 2026
Lands the deferred items from Round 2 PR #125's "follow-up" list. Each fix is small enough to review on its own; PR is structured as one commit because they all share a single migration window (off-chain BLS aggregator format change + new claim selector + struct field reorder already coordinated). CRITICAL — slash-and-dispute reentrancy (Round 2 economic F3) - `_settleDisputeBond` no longer pushes the bond back to the disputer on cancel. Instead the bond is credited to a per-disputer pull-pattern mapping (`_pendingDisputeBondRefunds`) and the disputer drains it via the new `claimDisputeBond()` selector. Closed the re-entrancy window where a contract-disputer's fallback could re-enter staking — at the bond-refund call point, `_operatorPendingSlashCount` has already been decremented, so an unstake call would slip past the slash-blocking gate and exit at the pre-slash exchange rate. - Treasury forfeit on execute remains push (treasury is protocol- controlled; no operator-side bypass possible there). - Added selectors: `claimDisputeBond()`, `pendingDisputeBondRefund(address)`, and `getSlashConfig()` (which was added in v0.13 but never registered on the slashing facet — the surface was unreachable on the proxy). HIGH — JobsAggregation snapshot binding (Round 2 operator-collusion #2c) - The BLS aggregation message now binds chainid, contract address, and a hash of the operator-set snapshot in addition to (serviceId, callId, output). A swap-and-pop reorder (operator leaves / forceRemove) invalidates any in-flight aggregated signature instead of silently mis-crediting a different operator at the same bitmap index. - BREAKING for off-chain aggregators: the message format now is `abi.encode("TANGLE_BLS_AGG_v1", chainid, address(tangle), serviceId, callId, keccak256(abi.encode(operators)), keccak256(output))`. - Test helpers updated; new `BLSTestHelper.buildJobResultMessage(serviceId, callId, tangleAddress, operators, output)` overload alongside the legacy 3-arg form (retained for tests that don't go through the on-chain aggregation path). HIGH — TangleToken burn disabled (Round 2 governance #5) - `burn(uint256)` and `burnFrom(address,uint256)` revert with `BurnDisabled()`. Unrestricted ERC20 burn allowed any holder to lower `getPastTotalSupply()` before a snapshot, deflating quorum and letting low-stake proposals pass. There is no protocol use case for users burning their TNT (inflation is governance-controlled via InflationPool). The internal `_burn` is retained for any future upgradeable code path that gates burn behind an explicit role. HIGH — proposal-action limit reduction (Round 2 governance #8) - `MAX_PROPOSAL_ACTIONS` lowered from 50 to 10. `MAX_ACTION_VALUE` lowered from 100k ETH to 10k ETH per action. 50 actions × 100k ETH was a vast surface area where a malicious proposer could bury a privileged call (`grantRole`, `upgradeToAndCall`, etc.) in action #50 of 50 where UI tooling may truncate / skim. Real legitimate proposals touch ≤ 5 targets. HIGH — fee-on-transfer rejection (Round 2 economic F6) - `PaymentLib.collectPayment` and `DepositManager._handleErc20Deposit` now reject FoT / rebasing tokens at ingress via balance-delta check. Without this, escrow / share accounting credits `amount` while the contract physically receives less, eventually bricking downstream transfers. Reverts with the new `FeeOnTransferTokenRejected(token)` error. MEDIUM — RebasingAssetAdapter virtual offset (Round 2 economic F2) - Apply VIRTUAL_SHARES=1e8 / VIRTUAL_ASSETS=1 (matching the staking-pool defenses) to the rebasing-adapter's deposit/withdraw share math. A one-wei seed plus a donation can no longer round a victim's later V-token deposit to zero shares. Adapter test fixtures updated for the new round-trip dust (~1 wei). MEDIUM — Arbitrum L2 fee refund leak (Round 2 cross-chain H-1) - New `setL2RefundAddress(address)` on `ArbitrumCrossChainMessenger`. Excess-fee and call-value refunds from `createRetryableTicket` route to the configured sweep address instead of the L1 caller's L2 alias (which has no receive logic, so refunds were permanently locked). MEDIUM — beacon BSM hook bypass (Round 2 cross-chain H-5, design note) - Documented intentional decision to NOT iterate the operator's blueprint list on `TangleL2Slasher.slashOperator`. Liquid-staking BSMs that need to react to beacon slashes must subscribe to the existing `BeaconSlashExecuted` event off-chain. On-chain enumeration would be O(N) gas-DoS — any single misbehaving BSM could grief every beacon slash on the chain. MEDIUM — missing __gap on rewards UUPS (Round 2 storage F-3) - Added `uint256[50] private __gap` to `TangleMetrics`, `RewardVaults`, `InflationPool`, `ServiceFeeDistributor`, `StreamingPaymentManager`. Without these, a future field addition risked colliding with newly- introduced parent contracts. LOW — ServiceFeeDistributor reentrancy review (Slither false positive) - Slither flagged `_claimAllForToken` as state-after-external-call inside loop. Verified non-exploitable: every reaching entry point (`claimFor`/`claimAll`/`claimAllBatch`) is `nonReentrant`. Documented inline. Tests - New `test/security/StorageLayoutSnapshotTest.t.sol` pins critical storage slot positions for `Tangle` and verifies the OZ ERC-7201 namespaced slots (`Initializable`, `AccessControl`, `ReentrancyGuard`) match v5.1.0. Round 2 storage F-1 / F-2 flagged upgrade-time field-reorder risks; this catches drift in CI. - `test_DisputeSlash_BondRefundedOnCancel` updated to drive the new pull-pattern flow. - 3 RebasingAdapter tests updated for virtual-offset math. - 3 BLS E2E tests updated for the new aggregation message format. Bindings v0.14.0 - Regenerated via `cargo xtask gen-bindings`. CHANGELOG details every fix and the BREAKING changes (burn disabled, proposal limits, aggregation message, FoT rejection). 1398/1398 forge tests pass on this branch (heavy fuzz / invariant suites excluded; same exclusions as Round 2 PR #125). Findings still architecturally pending (each genuinely needs design discussion, not in scope here): - Round 2 cross-chain C-3: receiver-rotation replay (epoch versioning) - Round 2 economic F5: billSubscription stake-JIT (TWAP design) - Round 1 staking S-1: LDV redeem controller scoping (interface change) - Round 1 staking S-2: registerAdapter migration logic - Round 1 G-02: ValidatorPodManager._slash share-based pool migration
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
Round 2 dispatched five adversarial red-team agents with explicit attacker mindsets (economic / MEV searcher, operator collusion, governance takeover, cross-chain bridge, storage / upgrade / init) plus a Slither static-analysis sweep. Found ~50 new attack surfaces beyond the eight Round 1 audits in PR #124. This PR lands the small-but-critical fixes that don't require interface-level redesign. Larger architectural changes are listed at the bottom for follow-up.
CRITICAL fixes
Beacon SSZ endianness (B-01) — mainnet blocker
getEffectiveBalanceGwei,getActivationEpoch,getExitEpoch,getWithdrawableEpoch, and_extractBalanceFromLeafnow perform the correct little-endian byte-swap on SSZ-packed uint64 fields. The consensus layer packs values into the HIGH 8 bytes of abytes32chunk in little-endian; the previous code read the LOW 64 bits as if big-endian, returning 0 (or a byte-swapped wrong value) for every effective balance, exit epoch, and validator balance proven from a real EigenPod-CLI fixture. Tests passed only because the in-tree fixtures mirrored the same wrong packing — fixtures are now SSZ-correct and a regression test pins the canonical 32-ETH leaf.TNTLockFactory delegate-on-init airdrop capture (gov #1)
getOrCreateLockrequiresmsg.sender == beneficiary. Without this gate, anyone could front-run the victim's first interaction with the deterministic lock, supply themselves asdelegatee, and persistently capture every future inbound TNT transfer's voting power.HIGH fixes
address requesteradded to the per-job RFQ typed data; previously any_permittedCallercould lift another's signed quote.verifyQuoteBatchrejectsrequester == address(0)outright.disputeSlashwindow extends throughexecuteAfter + TIMESTAMP_BUFFER. Prior asymmetry let a sequencer land an operator's dispute (revert) and 15s later anyone couldexecuteSlash.disputeResolutionDeadlinewindow AND capture the bond on auto-execution when treasury == admin.canSlash == falseso the bridge keeps the message available for retry.MEDIUM fixes
pinBlueprintrejects deprecated-grace revisions.minOperatorsunless BSM opts in via newforceRemoveAllowsBelowMin(serviceId)hook.Upgrade safety
Types.ServiceRequest.activatedmoved to the end of the struct so a hypothetical upgrade from a pre-activatedlayout can't read a non-zero byte from a different field asactivated == true.Test plan
forge buildcleantest/security/AuditFixesTest.t.sol, including admin self-dispute)test_extractBalance_RealSszLeaf_32ETHpins canonical SSZ uint64 decodingorigin/main: 2GovernanceUpgradeTest+ 1 unrelated)pricing-engine/tests/eip712_compat.rsMUST be regenerated to match before any quote-signing client ships.Findings deferred to follow-up PRs
Each is either too risky to bundle (architectural change) or needs design discussion:
onSlashbypass on beacon-slash path (liquid-staking accounting desync)RebasingAssetAdaptervirtual-offset migration (first-depositor inflation surface)cancelSlashbond-refund reentrancy via disputerbillSubscriptionarbitrage__gapon fiverewards/*UUPS implementationsERC20Burnableburn-before-snapshot quorum suppressionServiceFeeDistributor._claimAllForTokenreentrancy-eth pattern (state-after-external-call insidenonReentrantentry; needs CEI review)🤖 Generated with Claude Code