chore(audit): round 3 deferred fixes + bindings v0.14.0#126
Merged
Conversation
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
3 tasks
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 3 lands the deferred items from PR #125's "follow-up" list. Each fix is small enough to review on its own; bundled here because they all share the same migration window (off-chain BLS aggregator format change + new claim selector + struct field reorder already coordinated).
1398 / 1398 tests pass on this branch (heavy fuzz / invariant suites excluded; same exclusions as Round 2 PR #125). Bindings regenerated to v0.14.0.
Fixes by severity
CRITICAL
Slash-and-dispute reentrancy (Round 2 economic F3)
_settleDisputeBondno 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 newclaimDisputeBond()selector. Closes the re-entrancy window where a contract-disputer's fallback could re-enter staking — at the bond-refund call point,_operatorPendingSlashCounthad already been decremented, so an unstake call would slip past the slash-blocking gate and exit at the pre-slash exchange rate.HIGH
burnandburnFromrevert withBurnDisabled(). Previously any holder could lowergetPastTotalSupply()before snapshot to deflate quorum.MAX_PROPOSAL_ACTIONS50 → 10,MAX_ACTION_VALUE100k ETH → 10k ETH. Real legitimate proposals touch ≤ 5 targets; the wide bound let proposers bury privileged calls in action Update MSBM w/ new features in Tangle (same branch name) #50.PaymentLib.collectPaymentandDepositManager._handleErc20Depositnow reject FoT / rebasing tokens at ingress via balance-delta check.MEDIUM
setL2RefundAddress(address). Excess refunds fromcreateRetryableTicketno longer leak to the L1 caller's L2 alias.BeaconSlashExecutedoff-chain. On-chain enumeration would be O(N) gas-DoS.__gapon rewards UUPS (storage F-3) —uint256[50] private __gapadded toTangleMetrics,RewardVaults,InflationPool,ServiceFeeDistributor,StreamingPaymentManager.LOW
nonReentrant. Documented inline.Tests
test/security/StorageLayoutSnapshotTest.t.solpins critical storage slot positions forTangleand verifies OZ ERC-7201 namespaced slots match v5.1.0. Round 2 storage F-1 / F-2 flagged upgrade-time field-reorder risks; this catches drift in CI.test_DisputeSlash_BondRefundedOnCancelupdated to drive the new pull-pattern flow.Bindings v0.14.0
Regenerated via
cargo xtask gen-bindings. CHANGELOG entry covers every fix and explicitly enumerates the BREAKING changes (burn disabled, proposal limits, aggregation message, FoT rejection). New facet selectors registered:claimDisputeBond,pendingDisputeBondRefund,getSlashConfig(the v0.13getSlashConfigview was added but never wired on the slashing facet — making it unreachable on the proxy; fixed here).Off-chain consumer migration
tnt-core-bindings0.14.0(publish viacargo xtask publishafter merge)blueprintSDKtnt-core-bindings = "0.14.0"once published; update aggregator to use new BLS message formatdappyarn sync:tnt-core-assets; surfaceclaimDisputeBondin the dispute resolution UXdocsFindings still architecturally pending (out of scope here)
Each genuinely needs design discussion before implementation; not bundling forced-architectural changes into this PR.
🤖 Generated with Claude Code