fix(security): remediate all audit findings (1 critical + 21 high + medium/low), 1855 tests green

[drewstone]

Audit remediation — closes the pre-mainnet audit (#178)

Remediates the 1 critical + 21 high findings from the audit landed in #178.
Every PoC exploit was converted into a passing regression test that fails if the
fix is reverted (not deleted), and an independent adversarial verification pass
re-derived each fix against a residual-bypass checklist — which additionally caught
and closed one owner-gating gap (GovernanceDeployer.deployGovernance).

Test status: FOUNDRY_PROFILE=fast forge test → 1650 passed, 0 failed.

Critical

• Cross-chain bridge confused-deputy. BaseCrossChainMessenger /
  ArbitrumCrossChainMessenger.sendMessage were permissionless, letting an attacker
  forge a BEACON_SLASH under the adapter's trusted L1 identity and slash any operator
  with no beacon proof. Fixed with an owner/authorizedSenders allowlist +
  UnauthorizedSender revert. Regression: PoCBridgeForgery.

High (grouped)

• Slashing exposure (F-COORD-001): per-asset commitment exposure was applied twice
  → ~2× under-slash. Now applied exactly once at execute. (DoubleExposureUnderSlashPoC)
• Beacon accounting: pod-bounded slashBps (no cross-pod amplification),
  delegatedShares escrow + burn on undelegation (no phantom delegation), slash now
  reaches beacon-pool totalAssets (no no-op), validator-exit no longer misread as a
  slash, withdrawNonBeaconChainEth reserved-principal floor (no restaked-ETH drain).
  (PoCDoubleCount, PoCSlashNoop, PoCNonBeaconDrain, CrossChainSlashingTest)
• Oracles: UniswapV3Oracle uses Math.mulDiv and scales by 10^tokenDecimals
  once (no truncate-to-0 / 1e8× overvaluation); EIP4788Oracle genesis-relative
  timestamp flooring (valid ring-buffer key). (OraclePoC, OracleToUSDImpactPoC,
  L1OracleAuditPoC)
• Adapters: RebasingAssetAdapter symmetric 1e8/1e8 virtual offset
  (token-denominated 1:1, no inflated USD exposure); StandardAssetAdapter credits
  actual-received (fee-on-transfer safe). (RebasingShareScalePoC,
  FeeOnTransferAdapterPoC)
• RFQ / rewards: RFQ active-service-count increment (operators backing RFQ services
  are no longer falsely exitable) + startLeaving guard on all leave paths;
  RewardVaults _settle before score mutation (no top-up reward-debt theft) +
  commission cap/timelock. (RFQActiveServiceCountBypassPoC, F001_TopUpRewardDebtPoC)
• BLS / governance / lifecycle: BN254 rejects point-at-infinity / degenerate input;
  TNTCliffLock sealed impl + no auto-delegate; TNTLockFactory permissionless
  getOrCreateLock; GovernanceDeployer onlyOwner on every privileged-bootstrap
  entrypoint incl. deployGovernance (adversarial-pass catch);
  LiquidDelegationVault RateUndefined mint guard; ServicesLifecycle clears stale
  commitments on rejoin; PaymentsDistribution staker-share refund routing;
  BuybackBlueprintBase minOut slippage. (BN254Infinity, TNTLockupAuditPoC,
  test_DeployGovernance_RevertsForNonOwner)

Verification methodology

1. PoC flip: each baseline PoC demonstrated the live vuln, then was rewritten to
   assert the secure invariant — confirmed to fail with the fix reverted.
2. Adversarial pass: 8 read-only skeptics re-audited each (fix, test) pair for weak
   assertions and residual bypasses; findings cross-checked against the worktree
   (the false positives were stale reads of the pre-fix tree).

Rebased onto current main (incl. #179); no conflicts.

[tangletools]

✅ Auto-approved PR — 487b2e67

Blanket team auto-approval is enabled for this reviewer service.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

_{tangletools · auto-approval · reason: blanket_auto_approve · 2026-06-14T18:44:15Z}

[tangletools]

🟡 Value Audit — sound-with-nits

Verdict	sound-with-nits
Concerns	8 (1 medium-concern, 1 low, 6 weak-concern)
Heuristic	0.0s
Duplication	0.0s
Interrogation	582.0s (2 bridge agents)
Total	582.0s

💰 Value — sound-with-nits

A coherent, high-value pre-mainnet security remediation that closes the audited critical/high findings across beacon slashing, cross-chain messaging, staking accounting, oracles, buybacks, governance bootstrap, and reward vaults — ship after considering two small architectural cleanups.

• What it does: The change hardens the protocol against the 1 critical + 21 high findings from the prior audit. Concretely: (1) it restricts BaseCrossChainMessenger.sendMessage and ArbitrumCrossChainMessenger.sendMessage to an owner-configured authorizedSenders allowlist, closing the confused-deputy bridge-forgery path; (2) it re-derives the L2 slash amount in L2SlashingConnector from the slashed pod's ow
• Goals it achieves: The change makes the system safe to launch by eliminating forged cross-chain slashes, preventing under-slashing from double-counted exposure, stopping phantom/depleted delegations, protecting beacon principal from owner drain, fixing oracle price truncation/inflation, closing governance-bootstrap privilege escalation, blocking BLS universal forgery, and ensuring reward debt is not lost or retroact
• Assessment: The change is good on its merits. It is tightly scoped to the vulnerabilities it fixes, preserves existing selectors/storage layout where upgrade safety matters (e.g., ERC-7201 namespacing in L2SlashingReceiver, appended fields in RewardVaults.DelegatorDebt), and follows the project's share-based/virtual-offset accounting model. The inline invariant comments make the security reasoning auditab
• Better / existing approach: Two small cleanups are available, but no wholesale redesign or existing implementation should replace this work:

1. Cross-chain messenger authorization is duplicated between BaseCrossChainMessenger and ArbitrumCrossChainMessenger. Both manually implement owner, onlyOwner, transferOwnership, authorizedSenders, setAuthorizedSender, and UnauthorizedSender (`src/beacon/bridges/BaseCro

🎯 Usefulness — sound-with-nits

The audit remediation is coherently integrated into the live production paths and follows existing codebase patterns; the main reservations are operational/dead-code nits and a batch-propagation fee-forwarding flaw that only matters for fee-bearing bridges.

• Integration: The critical confused-deputy fix is wired end-to-end: BaseCrossChainMessenger.sendMessage is gated to owner/authorizedSenders (src/beacon/bridges/BaseCrossChainMessenger.sol:90-92), the deploy script authorizes the freshly deployed L2SlashingConnector (script/DeployBeaconSlashing.s.sol:190,444), and the production orchestrator runs that script (deploy/deploy-all.sh:63-74). The pod-scaled slash mat
• Fit with existing patterns: The fixes follow established patterns: the L1 adapter allowlist mirrors L2SlashingReceiver.authorizedSenders; slashBps uses the same BPS_DENOMINATOR convention as ValidatorPodManager._slash; the beacon-pool share escrow matches the share-based accounting already used for queued withdrawals; oracle math uses Math.mulDiv like ChainlinkOracle; and the slashing exposure split matches the existing two-
• Real-world viability: The production path (OP-Stack/Base, free L1 messaging, single pod per owner) is covered: single propagation forwards msg.value correctly, edge cases like operatorStake == 0 and slashBps capping are handled, and validator exits no longer trigger phantom slashes because effectiveCurrent adds back newlyWithdrawableGwei. The changes also guard withdrawNonBeaconChainEth against checkpoint-in-flight and

🔎 Heuristic Signals

🟡 Cruft: magic number added test/tangle/PerAssetExposureIntegration.t.sol

•    // commitment exposures (native 10000, ERC20 1000) are NOT folded in here — they
  

💰 Value Audit

🟡 Cross-chain messenger auth/ownership boilerplate is duplicated and manually reimplements OZ Ownable [better-architecture] ``

Both BaseCrossChainMessenger.sol:28-164 and ArbitrumCrossChainMessenger.sol:79-265 manually store owner, define onlyOwner with a require string, implement transferOwnership, store authorizedSenders, and emit AuthorizedSenderUpdated. The rest of the project imports OpenZeppelin Ownable for this (e.g., ValidatorPodManager.sol:33, UniswapV3Oracle.sol:6). Extracting an AuthorizedCrossChainMessenger abstract base would remove duplicated security-critical authorization code and

🟡 Pod-scaled slash math is copy-pasted across three functions in L2SlashingConnector [duplication] ``

The identical sequence podPrincipal * slashPercentage / 1e18, bps = podSlashAmount * 10_000 / operatorStake, cap at 10_000, and operatorStake * bps / 10_000 appears in _propagateBeaconSlashing (L2SlashingConnector.sol:266-285), getPendingSlashAmount (L2SlashingConnector.sol:346-354), and estimatePropagationFee (L2SlashingConnector.sol:393-405). A single internal _computeSlashBps helper would eliminate the duplication and guarantee the view/estimate/propagate paths stay in syn

🎯 Usefulness Audit

🟠 batchPropagateBeaconSlashing forwards zero value and reverts on fee-bearing bridges [robustness] ``

In src/beacon/L2SlashingConnector.sol:198 the batch loops try this.propagateBeaconSlashingInternal{value: 0}(...). The internal call reaches _propagateBeaconSlashing, which requires msg.value >= messenger.estimateFee(...) (:297-300). BaseCrossChainMessenger.estimateFee returns 0 (:123-125), so the current OP-Stack path works, but ArbitrumCrossChainMessenger.estimateFee returns submissionCost + l2ExecutionCost (:185-207), so every batch item reverts and is swallowed by the try/catch. Fix by

🟡 L1 cross-chain adapter ownership is not transferred in deploy scripts/runbook [integration] ``

BaseCrossChainMessenger sets owner = msg.sender in the constructor (:56-58) and DeployBeaconSlashing.s.sol only transfers ValidatorPodManager ownership (:175-178); deploy/RUNBOOK-launch.md also omits adapter ownership handoff. Since the security model expects the owner to be a timelock/multisig (BaseCrossChainMessenger.sol:54), add adapter ownership transfer to the deploy script and runbook.

🟡 ArbitrumCrossChainMessenger is not deployed or authorized by production scripts [integration] ``

DeployBeaconSlashing.s.sol defines BridgeProtocol with only OpStack (:29-31) and deployOpStackMessenger() (:184); no script deploys or authorizes ArbitrumCrossChainMessenger. The fix is correct but the path is dead until a deploy step is added. If Arbitrum support is not imminent, mark the contract as future-only to avoid implying it is production-ready.

🟡 UniswapV3Oracle is not deployed or configured by production scripts [integration] ``

UniswapV3Oracle is fixed and tested, but deploy scripts only read an existing PRICE_ORACLE address (script/Deploy.s.sol:352, script/FullDeploy.s.sol:483) and never deploy or reference the new oracle. The remediation is inactive until a deploy/runbook step wires it. Track as a follow-up.

🟡 TNTLockFactory retains unused NotBeneficiary error and ignores delegatee parameter [ergonomics] ``

TNTLockFactory.sol:70 keeps a NotBeneficiary error that is never reverted, and TNTCliffLock.sol:48 does not use the delegatee argument. This is intentional for ABI stability, but integrators may expect delegatee to set voting power. Add a clarifying comment or event note.

---

What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass	What it asks
Heuristic	Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication	Do added function/class names already exist elsewhere in the repo?
Value Audit	What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit	Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

_{value-audit · 20260614T185542Z}

[tangletools]

❌ Needs Work — 487b2e67

Readiness 0/100 · Confidence 95/100 · 39 findings (1 critical, 4 high, 7 medium, 27 low)

	deepseek	glm	aggregate
Readiness	0	0	0
Confidence	95	95	95
Correctness	0	0	0
Security	0	0	0
Testing	0	0	0
Architecture	0	0	0

Full multi-shot audit completed 8/8 planned shots over 46 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 46 changed files. Global verifier still owns final merge decision.

Blocking

🟣 CRITICAL Escrow burn creates phantom surplus drainable via withdrawNonBeaconChainEth, defeating service slash — src/beacon/ValidatorPodManager.sol

In completeUndelegation (lines 734-745), the escrow burn reduces bp.totalAssets (line 742: bp.totalAssets = burnAssets >= bp.totalAssets ? 0 : bp.totalAssets - burnAssets) but does NOT transfer any ETH out of the pod. completeUndelegation itself transfers zero ETH — it only updates delegation-pool and beacon-pool accounting. The physical ETH stays in the pod.

After the delegator completes their withdrawal (burning remaining beacon shares via completeWithdrawal, which DOES transfer ETH via withdrawToStaker), totalAssetsOf(p

🔴 HIGH Phantom escrow credit enabled theft of delegator staker pool — src/core/PaymentsDistribution.sol

At line 438, the _refundStakerShareToEscrow function previously checked only escrow.token != token. For non-subscription services (EventDriven/PayOnce), the escrow struct starts all-zero (totalDeposited==0, token==0). When _forwardStakerShare failed (no distributor configured, or distributor reverted), the old gate escrow.token(0) != token(0) was false, so escrow.balance was credited despite no real deposit backing it. The service owner could later call withdrawRemainingEscrow (PaymentsRefund.sol:35-54, which checks only ownership and terminated status — no pricing-model guard) to drain the phantom balance. The new gate `escrow.totalDeposited == 0 || escrow.t

🔴 HIGH Quote-service operators could unregister/withdraw stake while actively backing a live service — src/core/QuotesCreate.sol

At line 300, _processOperatorQuotes now increments _operatorActiveServiceCount[blueprintId][operators[i]]++. Previously this count was never incremented for quote-activated services, while the standard request/approve path in TangleServicesFacet.sol:140 did increment it. _operatorActiveServiceCount is consumed by two critical guards: unregisterOperator (Operators.sol:204) reverts if count > 0, and _startLeaving (OperatorManager.sol:248-257) reverts via staticcall to getOperatorTotalActiveServices. Without the increment, an operator backing a live quote service could unregister from the blueprint and withdraw their stake while still active. Verified: _terminateService (

🔴 HIGH Double-counting commitment exposure caused operator under-slashing — src/core/Slashing.sol

At lines 70-81, the removed _computeServiceCommitmentExposureBps averaged per-asset commitment.exposureBps and folded the average into effectiveSlashBps at propose time. Downstream, SlashingManager._slashForService:270 independently applies effectiveBps = (slashBps * commitment.exposureBps) / BPS_DENOMINATOR. Together, the realized slash per asset would be slashBps * opExposure * avg(commitmentExposures) * commitment.exposureBps / BPS_DENOMINATOR^3 instead of the correct slashBps * opExposure * commitment.exposureBps / BPS_DENOMINATOR^2 — systematically under-slashing the operator. The fix sets effectiveExposureBps = opData.exposureBps only ([line 81](https://gith

🔴 HIGH RebasingAssetAdapter virtual offset change is state-breaking — src/staking/adapters/RebasingAssetAdapter.sol

VIRTUAL_ASSETS changed from 1 to 1e8 (line 52), making the virtual offset symmetric with VIRTUAL_SHARES=1e8. Under the old asymmetric offset (1e8/1), existing depositors received 1e8x-scaled shares per token; under the new symmetric offset (1e8/1e8), shares are token-denominated 1:1. All view functions (sharesToAssets, assetsToShares, exchangeRate) now use the new formula, producing DIFFERENT results for legacy share balances. If the adapter already has deposits on any deployment, existing shareholders' positions are economically altered. The PR has an adapter migration system (AdapterMigrationInProgress errors in DelegationErrors.sol) but this shot'

Other

🟠 MEDIUM ValidatorPod effectiveCurrent fix enables slashing-factor suppression via direct ETH top-up — src/beacon/ValidatorPod.sol

The _finalizeCheckpoint change (line 463) computes effectiveCurrent = uint256(totalRestakedBalanceGwei) + uint256(newlyWithdrawableGwei). The intent is correct: normal validator exits (balance↓ matched by ETH arriving in pod) should not trigger a slash.

However, ValidatorPod accepts arbitrary ETH via receive() external payable {} (line 708). A pod owner who knows a beacon slash is imminent can send ETH directly to the pod before a checkpoint starts. This ETH increases _newlyWithdrawableGwei() ([line 673-676](https://github.com/tangle-net

🟠 MEDIUM No regression test for phantom-escrow staker-share drain fix — src/core/PaymentsDistribution.sol

Lines 425-441: The escrow.totalDeposited == 0 guard prevents a service owner from draining the staker pool on a native-token, non-subscription service (EventDriven/PayOnce). Verified: depositToEscrow (PaymentLib.sol:312) sets totalDeposited only on real funding; withdrawRemainingEscrow (PaymentsRefund.sol:45) sends escrow.balance to the service owner after termination. Without the guard, escrow.token == address(0) == token would pass the old check and credit a phantom balance the owner could withdraw. This is the most fund-safety-critical fix in this shot but has no dedicated PoC test. The SubscriptionEscrowInvariant fuzz test only covers funded su

🟠 MEDIUM No regression test for commitment-array clearing on rejoin — src/core/ServicesLifecycle.sol

Lines 204-225: The clearing loop deletes prior commitments and per-asset BPS entries before pushing new ones in joinServiceWithCommitments. Verified: _removeOperatorFromService (line 766) does NOT clear commitments, so a leave-then-rejoin would duplicate entries. _accrueOperatorWeights (PaymentsBilling.sol:274) iterates the array for billing weight, so duplication inflates the rejoined operator's share. No dedicated test exercises the leave → rejoin-with-commitments → verify-no-duplication flow. Without this test, a regression

🟠 MEDIUM Rejoining operator would duplicate security commitments, inflating billing weight and multiplying slash application — src/core/ServicesLifecycle.sol

At lines 204-225, joinServiceWithCommitments now clears _serviceSecurityCommitments and _serviceSecurityCommitmentBps before re-pushing commitments on a rejoin. Verified that _removeOperatorFromService (lines 766-795) does NOT clear these arrays — they persist across leave. Without this clearing, each rejoin would append to the existing array, doubling commitments each cycle. The impact: (1) _accrueOperatorWeights (PaymentsBilling.sol:274 iterate every commitment independently at [line 319-345](https://github.com/tangle-network/

🟠 MEDIUM Blueprint rebalance pending-slash guards lack direct regression tests — src/staking/DelegationManagerLib.sol

_addBlueprintToDelegation (line 865) and _removeBlueprintFromDelegation (line 938) both gate on _operatorPendingSlashCount[d.operator] > 0 to prevent slash evasion via stake rebalancing between blueprint pools. These are security-critical guards — without them, a Fixed-mode delegator whose operator has a pending slash could move stake out of the targeted blueprint pool and evade the slash entirely. BlueprintSelectionTest.t.sol tests add/remove flows but does not test the pending-slash revert scenario. No test in the test suite

🟠 MEDIUM ActiveServiceCheckUnavailable error path not directly tested — src/staking/OperatorManager.sol

The fail-closed staticcall guard at line 248-253 reverts with ActiveServiceCheckUnavailable when _tangleCore.code.length > 0 but the getOperatorTotalActiveServices(address) call reverts or returns short data. The RFQ bypass PoC (RFQActiveServiceCountBypassPoC.t.sol) tests the active-service counter increment on the TangleCore side but does not test the staking-side fail-closed behavior. No test wires a tangleCore contract that reverts on the selector (or returns no data) and asserts that _startLeaving() reverts with ActiveServiceCheckUnavailable. This is a P1 security guard — the absence of a regression test means a future refactor could silently revert to f

🟠 MEDIUM Storage slot discrepancy between test and TangleStorage.sol comments — test/audit/RFQActiveServiceCountBypassPoC.t.sol

Test line 36 hardcodes ACTIVE_SVC_COUNT_SLOT = 65 and reads the active-service counter directly via vm.load. However TangleStorage.sol:332 labels the _operatorActiveServiceCount mapping under 'Slot 121-125'. If the test slot is wrong, the regression guard reads the wrong storage location and could silently pass by coincidence (garbage == 1), making the security guard test worthless. The test comment says slot 65 was 'confirmed via forge inspect' but this must be independently reverified — a forge inspect Tangle storage output showing slot 65 would resolve this. Alternatively, replace the direct-storage read with a new public view function that retu

🟡 LOW L2SlashingConnector slashBps under-applies in multi-pod sequential delivery — src/beacon/L2SlashingConnector.sol

Each pod independently computes slashBps = podSlashAmount * 10_000 / operatorStake (line 280) against the CURRENT operatorStake at propagation time. When multiple pods slash the same operator, L2 applies each slashBps sequentially against the reducing stake (see MockStaking.slash at line 138: available * slashBps / 10_000). This means later pods' slashes are applied against an already-reduced base, under-slashing compared to the intended total. This errs in favor of the operator (safer direction than the over-slash the old cod

🟡 LOW PoCSlashNoop test does not verify surplus is not drainable after escrow burn — src/beacon/ValidatorPod.sol

The PoCSlashNoop test (test/beacon/PoCSlashNoop.t.sol) asserts received == 16.5 ether after completeWithdrawal but never checks that the remaining ~15.5 ETH in the pod is NOT drainable via withdrawNonBeaconChainEth. The test therefore passes despite the critical surplus-drain vulnerability (see finding above). A regression test should assert that pod.withdrawNonBeaconChainEth(podOwner, remainingBalance) reverts with InsufficientBalance after an escrow burn.

🟡 LOW setAuthorizedSender zero-address revert path untested on L1 messengers — src/beacon/bridges/ArbitrumCrossChainMessenger.sol

Both ArbitrumCrossChainMessenger.sol:120 and BaseCrossChainMessenger.sol:69 add require(sender != address(0), "Zero address") in setAuthorizedSender. The PoC test (PoCBridgeForgery.t.sol:153) verifies that unauthorized senders revert on sendMessage, but neither PoCBridgeForgery.t.sol nor CrossChainMessengersTest.t.sol tests the zero-address revert path in setAuthorizedSender. Impact: low — owner-gated admin function, zero-address is a no-op misconfiguration rather than an exploit vector. Add test_setAuthorizedSender_revertsOnZeroAddress in CrossChainMessengersTest.t.sol.

🟡 LOW BaseCrossChainMessenger.sendMessage uint32 truncation of buffered gas limit — src/beacon/bridges/BaseCrossChainMessenger.sol

Line 106: uint32(effectiveGasLimit) unsafe cast. _applyGasLimitWithBuffer can return values up to minGasLimit * (1 + gasBufferBps/10000). If gasBufferBps is set to 10000 (100% buffer), the result is 2 * minGasLimit. With default minGasLimit=100000, this is safe. But if minGasLimit is later admin-set near type(uint32).max / 2, truncation would silently reduce gas on L2. Pre-existing (not introduced here) but the new authorizedSenders gate ensures only the configured connector calls this, reducing attack surface. Low risk in practice; document the implicit gasLimit < 2^31 expectation.

🟡 LOW totalReleased flooring can break escrow accounting invariant in edge case — src/core/PaymentsDistribution.sol

Lines 443-447: When totalReleased < amount, the code floors totalReleased = 0 after incrementing balance += amount. This breaks the invariant balance + totalReleased == totalDeposited (balance exceeds totalDeposited). Pre-existing (not introduced by this PR), and in practice unreachable because _refundStakerShareToEscrow is always called after releaseFromEscrow which ensures totalReleased >= stakerShare. However, the else { escrow.totalReleased = 0; } branch is dead defensive code that masks a logic error if the call ordering ever changes. Consider reverting instead of flooring to fail loud.

🟡 LOW Comment references non-existent function _effectiveExposureBps in Slashing.sol — src/core/ServicesLifecycle.sol

Line 211 comment states: 'cannot leave a stale exposure that Slashing._effectiveExposureBps would still read.' However, Slashing.sol does not define a function named _effectiveExposureBps. The term effectiveExposureBps at Slashing.sol:81 is a local variable that reads opData.exposureBps, not the _serviceSecurityCommitmentBps mapping. The stale BPS mapping would still cause data inconsistency via the public view getServiceSecurityCommitmentBps (Base.sol:534), but the slashing-specific claim in the comment is technically inaccurate. A stale BPS entry is still harmful for other consumers — the comment should reference the actual reader path.

🟡 LOW Missing onlyOwner tests for configureProtocolRoles, transferTimelockAdmin, transferFullControl — src/governance/GovernanceDeployer.sol

Lines 174, 205, 216: These three functions are newly gated behind onlyOwner, but only deployGovernance's gate is tested (test_DeployGovernance_RevertsForNonOwner at test/Governance.t.sol:649). A non-owner revert test for each would confirm the access-control invariant the NatSpec at lines 41-46 claims. Fix: add 3 test cases pranking as a non-owner address, expecting NotOwner revert.

🟡 LOW No access-control revert tests for configureProtocolRoles/transferTimelockAdmin/transferFullControl — src/governance/GovernanceDeployer.sol

Lines 174, 205, 216: These three functions now carry onlyOwner but only deployGovernance has a dedicated test_DeployGovernance_RevertsForNonOwner test (Governance.t.sol:649). configureProtocolRoles and transferFullControl are exercised in integration tests but never directly tested for access control rejection. transferTimelockAdmin has zero test coverage. Pattern is identical to the tested deployGovernance case, so risk is low, but explicit coverage would strengthen the gate.

🟡 LOW Permissionless renounceTimelockAdmin enables griefing of multi-tx handover flows — src/governance/GovernanceDeployer.sol

Line 198: renounceTimelockAdmin has no onlyOwner. Anyone calling it triggers timelock.renounceRole(DEFAULT_ADMIN_ROLE, address(this)), which succeeds because OZ renounceRole checks account == msg.sender and both resolve to the deployer contract. A frontrunner observing a deployer contract between transactions can force premature admin renunciation, blocking subsequent transferTimelockAdmin/transferFullControl calls (they internally call timelock.grantRole which requires DEFAULT_ADMIN). Impact is griefing only — no privilege escalation, no fund theft, governance proposer/executor/canceller roles already configured remain intact. The DeployGovernance.s.sol sc

🟡 LOW Beneficiaries must manually delegate to activate locked TNT voting power — src/governance/lockups/TNTCliffLock.sol

Line 75: delegate() is onlyBeneficiary. After a batch distribution, locked TNT is transferred to the lock address but no delegation is set. Until the beneficiary calls lock.delegate(...), their locked voting power is unassigned (_delegatee[lock] == address(0)). This is the correct security tradeoff (prevents hijack) but constitutes a behavioral change from the old code where delegatee was auto-applied at init. Deployers should be aware that post-distribution, locked votes are inactive until beneficiaries explicitly delegate.

🟡 LOW delegatee event parameter now misleading for off-chain indexers — src/governance/lockups/TNTCliffLock.sol

Line 57: The delegatee parameter is still emitted in the Initialized event but is no longer acted upon (auto-delegation was removed). Similarly, LockCreated in TNTLockFactory.sol:62 emits delegatee but the lock does NOT delegate to it. Off-chain indexers that previously read these events and assumed lock._delegatee == event.delegatee will be wrong — the actual delegation state is address(0) until the beneficiary calls lock.delegate(). The events now record only a 'preferred delegatee hint'. Low severity because no on-chain logic relies on this; purely off-chain integration concern.

🟡 LOW Dead NotBeneficiary error in TNTLockFactory — src/governance/lockups/TNTLockFactory.sol

Line 70: error NotBeneficiary(address caller, address beneficiary) is declared but never reverted. Previously thrown by the removed msg.sender != beneficiary guard. Documented at lines 66-69 as retained for ABI/error-stability. This is intentional but could confuse auditors searching for where the error is thrown. Consider adding a forge-lint disable comment or moving to an errors-only file. No functional impact.

🟡 LOW LiquidDelegationVault _rateDefinedForMint may see stale totalAssets mid-transaction — src/staking/LiquidDelegationVault.sol

deposit() (line 217) calls _rateDefinedForMint() before external calls (safeTransferFrom, forceApprove, depositERC20, delegateWithOptions). If a slash or another pending redeem occurs between the gate check and the actual share minting (step 7, line 240), the rate could collapse mid-transaction. The depositor mints at the pre-collapse rate, effectively overpaying (benefiting existing holders). This is the correct economic outcome (no dilution) but it means the gate does not provide atomic protection — it only prevents mintin

🟡 LOW No test for RateUndefined revert path in LiquidDelegationVault — src/staking/LiquidDelegationVault.sol

Lines 213-217 and 250-252: the new _rateDefinedForMint() guard reverts with RateUndefined when totalAssets()==0 && totalSupply()>0. This is the slash-during-pending-redeem inflation-defense path. Existing vault tests exercise the clean-release case (test_Vault_SlashDuringPendingRedeem_ReleasesCleanely passes) but none trigger the deposit-blocked branch directly. A dedicated regression test should: (1) deposit into vault, (2) requestRedeem enough to set _pendingRedeemAssets close to underlying, (3) slash underlying below reservation, (4) assert deposit() reverts with RateUndefined, (5) claim redeems, (6) assert deposit() succeeds again. Without this

🟡 LOW No test for ActiveServiceCheckUnavailable fail-closed path in OperatorManager — src/staking/OperatorManager.sol

Lines 248-258: the changed _startLeaving() now reverts with ActiveServiceCheckUnavailable when _tangleCore.code.length > 0 but the staticcall fails or returns < 32 bytes. This is the critical fix replacing the old fail-open behavior. No test exercises this path. A regression test should mock a _tangleCore contract whose getOperatorTotalActiveServices reverts (or returns short data) and assert _startLeaving reverts with ActiveServiceCheckUnavailable rather than silently passing. The RFQActiveServiceCountBypassPoC tests the counter-increment fix in Tangle core but not the staking-side fail-closed guard itself.

🟡 LOW RebasingAssetAdapter assetsToShares removes currentBalance==0 guard — src/staking/adapters/RebasingAssetAdapter.sol

The old assetsToShares (line 193 removed) had if (currentBalance == 0) return 0 — when the pool held zero tokens but had positive shares (degenerate post-slash or post-drain state), the function returned 0, preventing anyone from quoting a non-zero share count. The new formula (line 194) returns assets * (totalShares + VIRTUAL_SHARES) / (0 + VIRTUAL_ASSETS) > 0 in this case, which could present stale preview values. However, the actual deposit() function measures balance-before/after and correctly compute

🟡 LOW RebasingAssetAdapter sharesToAssets behavior change on empty pool — src/staking/adapters/RebasingAssetAdapter.sol

Line 182: changed guard from if (totalShares == 0) return 0 to if (shares == 0) return 0. This means sharesToAssets(X>0) on an empty pool now returns X (via the virtual-offset formula: X * (0+1e8)/(0+1e8) = X) instead of 0. This is correct per ERC4626 convertToAssets semantics and matches the withdraw path (which would revert via InsufficientAssets check anyway). Low-risk behavioral change but worth noting that any downstream caller that previously received 0 for sharesToAssets on an empty pool will now get a non-zero value.

🟡 LOW RebasingShareScale test only covers bootstrap deposit, not post-rebase share accuracy — test/adapters/RebasingShareScalePoC.t.sol

Both tests (test_RebasingDeposit_Mints_1To1_TokenWei and test_PreviewDeposit_Agrees_With_Deposit) only verify the first-depositor bootstrap case (totalShares=0, balance=0). The symmetric offset also governs share math after the pool has assets and the rebasing token has changed balance, but that path is untested here. Not a bug — the scope matches F-003/F-004 — but a second deposit after a simulated rebase would harden the guard against reintroduced asymmetry in the non-bootstrap branch.

🟡 LOW BN254 test does not cover real-sig + infinity-pubkey case — test/audit/BN254Infinity.t.sol

The production guard at BN254.sol reverts on isG1Infinity(signature) || isG2Infinity(pubkey). The tests cover (infinity-sig, infinity-pubkey) and (infinity-sig, real-pubkey), but not (real-sig, infinity-pubkey). The isG2Infinity branch is only exercised transitively in test 1 where both inputs are zero. A dedicated case with a nonzero G1 signature and G2-infinity pubkey would directly assert the isG2Infinity arm, closing the coverage gap for rogue-key-style aggregation forgeries. Impact: low — production code handles it correctly via the || short-circuit.

🟡 LOW Hardcoded storage slot 65 is fragile across layout changes — test/audit/RFQActiveServiceCountBypassPoC.t.sol

Line 36: ACTIVE_SVC_COUNT_SLOT = 65 is a magic number tied to the current TangleStorage layout (TangleStorage.sol:337). If any storage variable is inserted before _operatorActiveServiceCount, the slot shifts and the test silently reads the wrong slot — it would either pass vacuously (reading 0 from an unrelated empty slot) or fail with a confusing assertion message. Consider adding a forge-inspect-generated constant comment or a runtime sanity check (e.g., assert the slot is nonzero after a known increment on the standard path, which the control test already does implicitly). Impact: low — test-only, currently correct, but a future storage migratio

🟡 LOW RFQ test does not verify counter decrements on service termination — test/audit/RFQActiveServiceCountBypassPoC.t.sol

The test asserts the counter is incremented (line 81) and unregister is blocked (line 86-87), but never terminates the service to verify the counter returns to 0 and the operator can subsequently unregister. If a future change broke the decrement path in _terminateService (ServicesLifecycle.sol:130-131) specifically for RFQ-originated services, this regression guard would not catch the permanent-lock bug. Consider extending with a post-termination assertion. Impact: low — the decrement code path is shar

🟡 LOW uint256 return from vm.load for uint32 storage value — test/audit/RFQActiveServiceCountBypassPoC.t.sol

Line 55: _readActiveServiceCount casts vm.load result to uint256 but the storage variable is uint32. The upper 28 bytes read from storage could contain residual data if the slot is shared with another packed variable. Currently this is safe because uint32 at a mapping value location is not packed with adjacent values, but the type mismatch is fragile. Use vm.loadUint or mask to uint32 for correctness.

🟡 LOW Stale operatorDelegatedStake mock in test_batchPropagateBeaconSlashing (non-MultiplePods variant) — test/beacon/CrossChainSlashingTest.t.sol

Line 586-611 (test_batchPropagateBeaconSlashing, the non-MultiplePods variant) propagates two pods without _mockPodPrincipal. Both pods were created via new MockSlashPod (not podManager.createPod()), so podToOwner returns address(0) and totalAssetsOf(0) returns 0, yielding slashBps=0 in both messages. The test only asserts messageCount == 2, which passes, but doesn't validate slash amounts or payload correctness. The test_batchPropagateBeaconSlashing_MultiplePods variant at line 425 does correctly

🟡 LOW Stale operatorDelegatedStake mock in test_estimatePropagationFee_UsesMessengerQuote — test/beacon/CrossChainSlashingTest.t.sol

Line 491-495 mocks operatorDelegatedStake instead of calling _mockPodPrincipal. The connector now uses getOperatorStake (not operatorDelegatedStake), podToOwner, and totalAssetsOf for slash computation in estimatePropagationFee. Without _mockPodPrincipal, podToOwner(pod1) returns address(0), totalAssetsOf(0) returns 0, so podPrincipal=0 and slashBps=0. The test passes because it only asserts fee == quotedFee, but the payload being estimated carries a degenerate slashBps=0 that doesn't exercise the hardened connector math. Fix: replace the operatorDelegatedStake mock with `_mockPodPrincipal(pod1, operator1, somePodPrincipal, som

🟡 LOW Bare vm.expectRevert() instead of specific error selector in forged-delivery test — test/beacon/PoCBridgeForgery.t.sol

Line 174 uses vm.expectRevert() without a selector for test_ForgedDelivery_FromNonAdapter_IsRejected. The receiver reverts with UnauthorizedOpStackSender(sourceChainId, xDomainSender) (L2SlashingReceiver.sol:312), so the test could assert the specific selector. A bare expectRevert would also pass if the contract reverted for an unrelated reason (e.g. a future bug that reverts on a different check). Minor precision loss; fix: vm.expectRevert(abi.encodeWithSelector(L2SlashingReceiver.UnauthorizedOpStackSender.selector, srcChainId, attacker)).

🟡 LOW Misleading tick price comment: says ~3000, actual is ~3290 — test/oracles/OraclePoC.t.sol

Line 69-71 comment states 'tick for price ~3000 USDC(6dec)/WETH(18dec)' with calculation showing raw price = 3e-9, but tick -195331 actually yields 1.0001^(-195331) ≈ 3.291e-9, producing ~3290.84 USD/WETH (as correctly asserted on line 83). The exact assertion value is correct; only the comment's '~3000' approximation is misleading. Fix: update comment to say '~3290' or remove the approximate human-price annotation since the exact value is asserted.

🟡 LOW Redundant assertion: line 118 restates line 115 — test/oracles/OraclePoC.t.sol

Line 115 asserts data.price == correct (1.8e9). Line 118 asserts data.price == 1.8e15 / 1e6 which evaluates to 1.8e9 — identical check. Intentional as documentation of the invariant ('reintroducing the inverted scaling re-inflates by exactly 1e6'), but functionally provides zero additional coverage. Not a bug; consider replacing with a structural assertion that the old buggy formula would produce a different value.

---

_{tangletools · 2026-06-14T20:13:12Z · trace}

[tangletools]

❌ 5 Blocking Findings — 487b2e67

Full multi-shot audit completed 8/8 planned shots over 46 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 46 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary

---

_{tangletools · 2026-06-14T20:13:12Z · immutable trace}

[tangletools]

✅ Auto-approved PR — aad03478

Blanket team auto-approval is enabled for this reviewer service.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

_{tangletools · auto-approval · reason: blanket_auto_approve · 2026-06-15T10:50:55Z}

[tangletools]

🟡 Value Audit — sound-with-nits

Verdict	sound-with-nits
Concerns	7 (2 low, 5 weak-concern)
Heuristic	0.2s
Duplication	0.0s
Interrogation	333.1s (2 bridge agents)
Total	333.3s

💰 Value — sound-with-nits

A large, coherent pre-mainnet security remediation that fixes confused-deputy cross-chain slashing, beacon/L2 slash accounting, oracle manipulation/griefing vectors, and governance/delegation edge cases, ships with regression tests, and keeps 1855 tests green; only minor maintainability nits from du

• What it does: The PR patches the 1 critical + 21 high audit findings from #178 across the beacon bridge (Base/Arbitrum L1 messengers now restrict sendMessage to owner/authorizedSenders), L2 slashing connector/receiver (pod-scaled slash bps, deferred-slash debt bank), ValidatorPod/ValidatorPodManager (pod-bounded slash, delegated-share escrow+burn, non-beacon-ETH floor), oracles (UniswapV3 full-precision pricing
• Goals it achieves: Eliminate the critical confused-deputy beacon-slash forgery path, remove double-counting in slashing/exposure math so operators are neither over- nor under-slashed, make cross-chain and oracle prices fail-closed against manipulation, griefing, and sequencer outages, prevent front-running/squatting of operator keys and quote signatures, stop delegators from withdrawing slashed principal or dodging
• Assessment: Good change. The fixes are root-cause-oriented (fail-closed before state mutation, bank deferred debt instead of reverting, scale slashes to the pod's contribution), fit the existing diamond/facet and ERC-7201 namespaced-storage patterns, and are backed by regression tests. The PR is large but the blast radius is justified by the audit scope, and the changes are surgical rather than architectural
• Better / existing approach: No materially better overall architecture — the remediation is the right shape. However, there are several places where existing code or a shared abstraction should be reused instead of duplicating logic: (1) StakingDelegationsFacet duplicates DelegationManagerLib._settleDelegatedCostBasis because the library function is private; (2) UniswapV3Oracle recreates ChainlinkOracle's sequencer-uptime int

🎯 Usefulness — sound-with-nits

Broad, well-integrated audit remediation that wires the cross-chain beacon-slash path into existing diamond/UUPS/ERC-7201 architecture and leaves only minor dead-code cleanups.

• Integration: The new behavior is wired and reachable. The L1→L2 beacon slash path is: off-chain oracle → L2SlashingConnector.propagateBeaconSlashing (deploy/LAUNCH-READINESS.md:71; deploy/config/base-mainnet.json:259 lists the oracle watcher as the remaining operational TODO) → Base/ArbitrumCrossChainMessenger.sendMessage (deploy scripts authorize the connector at script/DeployBeaconSlashing.s.sol:190, 444) →
• Fit with existing patterns: It follows the codebase's established patterns rather than competing with them: diamond facets for staking slashing (StakingSlashingFacet wraps the existing SlashingManager._snapshotOperator with a write-once guard), ERC-7201 namespaced storage in L2SlashingReceiver, UUPS proxies, and the existing IStaking.slash() interface via TangleL2Slasher. The bridge adapters keep the same adapter→receiver sh
• Real-world viability: The design handles realistic failure modes: L2SlashingReceiver banks deferred slash bps when canSlash is false (paused / no stake) instead of dropping the slash, capped at 10_000 bps per flush (L2SlashingReceiver.sol:451-467). L2SlashingConnector bounds each pod's L2 slash to that pod's beacon principal and re-expresses it against the operator's total L2 stake, preventing multi-pod amplification (

🔎 Heuristic Signals

🟡 Cruft: commented out code test/audit/medlow/SlashingCore.t.sol

•    // Let the dispute auto-fail (deadline passes), then execute the slash, forfeiting
  

🟡 Cruft: magic number added test/audit/medlow/DelegationLib.t.sol

• uint16 internal constant OPERATOR_COMMISSION_BPS = 1000;

🎯 Usefulness Audit

🟡 Unreachable operator==address(0) checks in L2SlashingConnector [ergonomics] ``

_getOperatorForPod reverts UnknownPod when podOperator[pod] is zero (L2SlashingConnector.sol:487-493), so the subsequent if (operator == address(0)) return in _propagateBeaconSlashing (L2SlashingConnector.sol:255-257) and the ternary operator == address(0) ? 0 : ... in estimatePropagationFee (L2SlashingConnector.sol:417) are dead code. Remove them or change _getOperatorForPod to return address(0) to keep the defensive intent.

💰 Value Audit

🟡 Facet-local copy of DelegationManagerLib._settleDelegatedCostBasis [duplication] ``

StakingDelegationsFacet.sol:280-315 contains an inline _settleDelegatedCostBasisInFacet that mirrors DelegationManagerLib.sol:664-696 line-for-line. The only reason for the copy is that the library function is private; making it internal would let the facet call the single source of truth and avoid future drift.

🟡 UniswapV3Oracle reinvents ChainlinkOracle's sequencer-uptime guard [duplication] ``

ChainlinkOracle.sol:325-335 already has ISequencerUptimeFeed + _requireSequencerUp. UniswapV3Oracle.sol:123-129 and :419-430 duplicates the same interface and function. Extract a shared SequencerUptime library or internal helper so both oracles stay consistent if the check ever changes.

🟡 Base and Arbitrum cross-chain adapters duplicate authorization/gas-buffer code [better-architecture] ``

Both BaseCrossChainMessenger.sol (owner/authorizedSenders at lines 39-56, setAuthorizedSender at 84-90, sendMessage gate at 104-110, gas config at 157-172) and ArbitrumCrossChainMessenger.sol (owner/authorizedSenders at 94-111, setAuthorizedSender at 132-138, sendMessage gate at 152-158, gas config at 237-252) independently implement the same authorization and gas-buffer machinery. A shared abstract CrossChainMessengerBase would remove duplicated boilerplate and prevent the two adapters from d

🟡 EIP-4788 has-root check duplicated across oracle and relayer [duplication] ``

EIP4788Oracle.sol:60-63 and BeaconRootRelayer.sol:121-123 both implement the same success && returndata.length == 32 check against the EIP-4788 precompile. A small BeaconRoots library/internal helper would centralize the precompile call semantics.

---

What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass	What it asks
Heuristic	Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication	Do added function/class names already exist elsewhere in the repo?
Value Audit	What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit	Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

_{value-audit · 20260615T105816Z}