chore(v0.17.1): audit batch — reports + stress harness + remediation by drewstone · Pull Request #144 · tangle-network/tnt-core

drewstone · 2026-05-16T22:43:37Z

Consolidates the four 2026-05-16 red-team audit reports, the local stress harness, and remediation fixes for 2 HIGH and 5 MEDIUM findings into a single batch. Supersedes #139 #140 #141 #142 #143.

Audit reports landed (`docs/audits/REDTEAM-*-2026-05-16.md`)

Audit	H	M	L	I
Reentrancy + griefing	0	3	2	1
Economic + oracle	1	3	2	2
DoS + access control	2	4	4	3
Storage + UUPS upgrade	0	0	1	3
Total	3	10	9	9

Stress harness landed

scripts/local-env/stress-test.sh drives a 17-step end-to-end flow against a local anvil node: deploys protocol, registers operators, opens services, funds escrow, fires bills, exercises slashing, tests the griefing-token path via script/StressGriefingSeed.s.sol + anvil_setStorageAt, asserts final state. 17/17 green across five consecutive runs. Runbook in STRESS-TEST.md.

Remediations in this PR

HIGH — DoS H-1

Cap customer-supplied security-requirement arrays at MAX_SECURITY_REQUIREMENTS_PER_REQUEST = 16 in _validateSecurityRequirements. An unbounded array let a customer brick their own subscription bills by forcing the per-bill O(operators × requirements) walk past the block gas limit.

HIGH — DoS H-2 (also closes Reentrancy M-3)

Every bare try IBlueprintServiceManager(...).fn() catch callsite now routes through _tryStaticcallManager(addr, calldata, minReturnLen) which forwards exactly MANAGER_HOOK_GAS_LIMIT (500_000) gas. Without the cap, Solidity's try/catch forwards 63/64 of remaining gas — a malicious BSM could drain the keeper's budget.

Sites converted: querySlashingOrigin, requiresAggregation (×2), getNonPaymentTerminationPolicy, canJoin, canLeave, forceRemoveAllowsBelowMin, getExitConfig, getMinOperatorStake (×2), getHeartbeatInterval, getHeartbeatThreshold, getRequiredResultCount, queryIsPaymentAssetAllowed, getAggregationThreshold. onAggregatedResult (mutating) routed through _tryCallManager.

MEDIUM — Reentrancy M-1

Developer / TNT-discount / treasury push transfers in _distributeBill wrapped in PaymentLib.tryTransferPayment. On failure, the un-sent amount folds into the operator pool and emits PushTransferFailed with a structured destination tag (developer / tnt-discount / treasury).

MEDIUM — Economic M-1

oracle.toUSD on the billing hot path wrapped in _safeToUSD / _safeToUSDView helpers (gas-capped at ORACLE_QUERY_GAS_LIMIT = 250_000) with raw-amount fallback + PriceOracleFallback event. A stalled or reverting oracle now degrades to raw token-second weighting instead of freezing all bills for the configured assets.

LOW — Storage L-1

MultiAssetDelegation._authorizeUpgrade now requires UPGRADER_ROLE (was ADMIN_ROLE). Role added to the initializer.

Greenfield cleanup

All M-N FIX: / H-N FIX: / Round 4 audit S-N: / G-02 follow-up: / C-3 (Round 4): audit-round tag comments stripped across 30 files. Descriptive content that explains current behavior is retained; historical narrative is deleted.
ValidatorPodManager._legacy* mappings (and the comments claiming they were preserved "for storage layout") deleted entirely. VPM is constructor-deployed Ownable, not UUPSUpgradeable, so the slot-preservation claim was wrong anyway.
DepositManager and StakingDelegationsFacet currentDeposits clamps collapsed to checked subtraction (the defensive case is structurally unreachable).

Fuzz coverage

test/tangle/SubscriptionEscrowInvariant.t.sol grew two invariants and an adversarial actor:

invariant_billAmountNeverExceedsNominalRate — catches a regression of the cap-at-nominal clamp under stake-ramp sequences.
invariant_baselinePinnedAtActivation — catches any post-activation code path that re-pins subscriptionBaselineStake.
stakeRamper handler actor that depositAndDelegates / schedules unstakes against the operator during the run.

What's deliberately deferred

HIGH Economic H-1 (oracle weight inflation): the proper fix is to snapshot per-(op, asset) USD prices at activation and reuse them at every bill, mirroring the baseline pin. Touches TangleStorage + PaymentsBilling._accrueOperatorWeights + PaymentsDistribution._initSubscriptionBaseline + ServicesLifecycle._finalizeJoin. Worth its own focused PR with negative-tested invariants. Customer is safe today (cap-at-nominal); residual risk is distributional between operators in the same service.
MEDIUM Reentrancy M-2 (escrow rescue path): admin-rescue route for stuck escrow tokens when the customer's token is centrally paused / blocklists the service owner. Moderate scope; better as its own PR.

Test plan

FOUNDRY_PROFILE=local_build forge build clean
FOUNDRY_PROFILE=fast forge test regression (non-integration, non-invariant)
scripts/local-env/stress-test.sh 17/17 green
Manual review of HIGH/MEDIUM remediations before merge
Follow-up PR for HIGH Economic H-1 + MEDIUM Reentrancy M-2

3 medium / 2 low / 1 informational. No critical or high-severity issues. Medium: `_distributeBill` push to BSM-resolved developer recipient can be permanently griefed by a malicious manager; `withdrawRemainingEscrow` has no fallback when the escrow token is broken; `getNonPaymentTerminationPolicy` is invoked without a gas cap, letting a malicious BSM grief the livelock escape. Low: `_settleDisputeBond` strands dispute bonds on executed proposals when the treasury push fails; `_operatorActiveSlashProposals` cap check ordering is inverted (cosmetic). Confirmed clean: every token-moving entry point holds `nonReentrant`; the OpenZeppelin ReentrancyGuard storage slot is genuinely shared across facets via ERC-7201; operator and keeper payments are pull via `_pendingRewards`; self-call gates fire before any state read; CEI ordering holds on every withdrawal / claim / refund path; the subscription livelock escape (`terminateServiceForNonPayment`) is permissionless.

Read-only audit of subscription billing, share-pool inflation defense, slashing replay, oracle adapters, and TWAP weighting under adversarial sequences. Customer is protected by cap-at-nominal; per-operator weight split is the residual attack surface. Severity counts: 0 CRITICAL, 1 HIGH, 3 MEDIUM, 2 LOW, 2 INFORMATIONAL. Top findings: - H-1: oracle-manipulated weight inflation captures the operator pool share of a (capped) bill, redistributing rent from honest operators - M-1: oracle revert during billing bricks subscriptions for the configured assets (no fund loss, denial-of-billing) - M-3: stake-ramp at periodEnd inflates within-period TWAP weight beyond the operator's time-averaged backing

Read-only static audit of DoS surfaces and access-control gaps across src/core/, src/staking/, and src/beacon/. Two HIGH findings on the permissionless billing path: unbounded security-commitment arrays brick subscription billing, and try IBlueprintServiceManager(...) callsites forward 63/64 of remaining gas instead of capping at MANAGER_HOOK_GAS_LIMIT. Four MEDIUM and four LOW notes plus 12 clean checks.

Single-file harness at scripts/local-env/stress-test.sh that boots a fresh anvil + LocalTestnet deployment and walks 17 ordered economic checks against the merged-PR surface (#132 subscription billing rearchitecture, #133 multi-asset bill weighting + EIP-170 facet split, #134 O(1) operator stake aggregate + share-pool slashing, #136 claimRewardsAll griefing isolation, #138 indexer event handlers). Highlights: - Idempotent: clean cleanup of anvil, broadcast artifacts, indexer state. - Per-step pass/fail with timing + headline metric. Single-line summary. - Optional --with-indexer / --with-dapp / --with-operator flags for the off-chain side processes; none required for the 17-step protocol surface. - Griefing-token isolation step deploys a RevertingTransferERC20 and seeds Tangle's _pendingRewards + _pendingRewardTokens AddressSet via anvil_setStorageAt (vm.store from a broadcast script doesn't propagate to anvil; the harness drives the seed via curl directly). Companion docs at scripts/local-env/STRESS-TEST.md cover prereqs, per-step PR mapping, log locations, debugging recipes, and an extension guide. A full green run takes ~40-85s on a warm-cache checkout.

Consolidates the four 2026-05-16 red-team audit reports, the local stress harness, and remediation fixes for 2 HIGH and 5 MEDIUM findings into a single batch. Originally PRs #139 #140 #141 #142 #143 — supersedes those. ## Audit reports landed (docs/audits/REDTEAM-*-2026-05-16.md) - Reentrancy + griefing: 0 H / 3 M / 2 L / 1 Info - Economic + oracle: 1 H / 3 M / 2 L / 2 Info - DoS + access control: 2 H / 4 M / 4 L / 3 Info - Storage + UUPS upgrade: 0 H / 0 M / 1 L / 3 Info ## Stress harness landed `scripts/local-env/stress-test.sh` drives a 17-step end-to-end flow against a local anvil node: deploys the protocol, registers operators, opens services, funds escrow, fires bills, exercises slashing, tests the griefing-token path via `script/StressGriefingSeed.s.sol` + `anvil_setStorageAt`, and asserts final state. 17/17 green across five runs. Runbook in `STRESS-TEST.md`. ## Remediations HIGH — DoS H-1: cap customer-supplied security-requirement arrays at `MAX_SECURITY_REQUIREMENTS_PER_REQUEST = 16` in `_validateSecurityRequirements`. An unbounded array let a customer brick their own subscription bills by forcing the per-bill `O(operators × requirements)` walk past the block gas limit. 16 is well above any realistic heterogeneous-asset blueprint and keeps worst-case `64 × 16 = 1024` inner iterations within one block. HIGH — DoS H-2 (also closes Reentrancy M-3): every bare `try IBlueprintServiceManager(bp.manager).fn() catch` callsite now routes through `_tryStaticcallManager(addr, calldata, minReturnLen)` which forwards exactly `MANAGER_HOOK_GAS_LIMIT` (500_000) gas. Without the cap, Solidity's `try/catch` forwards 63/64 of remaining gas, letting a malicious BSM drain the keeper/proposer's budget. Covers `querySlashingOrigin`, `requiresAggregation` (×2), `getNonPaymentTerminationPolicy`, `canJoin`, `canLeave`, `forceRemoveAllowsBelowMin`, `getExitConfig`, `getMinOperatorStake` (×2), `getHeartbeatInterval`, `getHeartbeatThreshold`, `getRequiredResultCount`, `queryIsPaymentAssetAllowed`, `getAggregationThreshold`, and converts the mutating `onAggregatedResult` hook to `_tryCallManager`. MEDIUM — Reentrancy M-1: developer / TNT-discount / treasury push transfers in `_distributeBill` wrapped in `PaymentLib.tryTransferPayment`. On failure, the un-sent amount folds into the operator pool and emits `PushTransferFailed` with a structured destination tag. A malicious BSM-resolved developer recipient (or paused/blocklisting token) can no longer brick distribution for honest operators. MEDIUM — Economic M-1: `oracle.toUSD` on the billing hot path wrapped in `_safeToUSD` / `_safeToUSDView` helpers (capped at `ORACLE_QUERY_GAS_LIMIT = 250_000`) with raw-amount fallback + `PriceOracleFallback` event. A stalled or reverting oracle now degrades to raw token-second weighting instead of freezing all bills. `_accrueOperatorWeights` drops `view` since the fallback path emits. LOW — Storage L-1: `MultiAssetDelegation._authorizeUpgrade` now requires `UPGRADER_ROLE` (was `ADMIN_ROLE`), restoring the defense-in-depth role separation the protocol-level contracts already use. Role added to the initializer. ## Greenfield cleanup The pre-launch protocol carried audit-round tag comments left over from prior remediation rounds — `M-8 FIX:`, `H-1 FIX:`, `Round 4 audit S-1:`, `G-02 follow-up:`, `C-3 (Round 4):`. Stripped across 30 files. Descriptive content that explains current behavior is retained; historical narrative is deleted. VPM also carried `_legacy*` mappings with comments suggesting they were kept for "storage layout preservation" in a contract that turns out not to be upgradeable (`ValidatorPodManager` is constructor-deployed `Ownable`, not `UUPSUpgradeable`). The mappings and misleading comments are deleted entirely. Also: the `if (config.currentDeposits >= amountReturned) { … } else { … clamp to 0 }` patterns in `DepositManager` and `StakingDelegationsFacet` are collapsed to a single checked subtraction. The clamp described a defensive case that is structurally unreachable under the current accounting. ## Fuzz coverage `test/tangle/SubscriptionEscrowInvariant.t.sol` grew two new invariants and an adversarial actor: - `invariant_billAmountNeverExceedsNominalRate` — catches a regression of the cap-at-nominal clamp under adversarial stake-ramp sequences. - `invariant_baselinePinnedAtActivation` — catches any post-activation code path that re-pins `subscriptionBaselineStake`. - `stakeRamper` handler actor that `depositAndDelegate`s / schedules unstakes against `operator1` during the run. ## What's deliberately deferred to a follow-up - HIGH Economic H-1 (oracle weight inflation): the proper fix is to snapshot per-(op, asset) USD prices at activation and reuse them at every bill, matching the baseline pin. The fix is architecturally involved (touches `TangleStorage`, `PaymentsBilling._accrueOperatorWeights`, `PaymentsDistribution._initSubscriptionBaseline`, `ServicesLifecycle._finalizeJoin`) and warrants its own focused PR with negative-tested invariants. The customer is safe today (cap-at-nominal bounds total damage); the residual risk is distributional between operators in the same service. - MEDIUM Reentrancy M-2 (escrow rescue path): admin-rescue route for stuck escrow tokens when the customer's token is centrally paused / blocklists the service owner. Moderate scope; better as its own PR. Supersedes #139 #140 #141 #142 #143.

tangletools · 2026-05-16T22:49:22Z

✅ No Blockers — `040d435d`

Readiness 72/100 · Confidence 78/100 · 8 findings (4 medium, 4 low)

	kimi-code	verifier:glm	aggregate
Readiness	78	72	72
Confidence	93	78	78
Correctness	78	78	78
Security	76	75	75
Testing	82	75	75
Architecture	84	85	84

Reviewed all 54 changed files, ran both modified test suites (22 unit tests + 3 invariant tests, all passing), and traced cross-contract call graphs. The PR is a solid audit remediation that correctly gas-caps manager hooks and oracle calls, adds best-effort payment transfer isolation, and removes dead storage. Three medium-severity gaps remain: the slashing path still uses an un-capped oracle call, two staking contracts lost a defensive accounting clamp, and the UPGRADER_ROLE migration needs explicit runbook documentation for live proxies. | Verified 8 findings from kimi-code (all valid). The

🟠 MEDIUM Slashing path missed oracle hardening — src/core/Slashing.sol

Line 190: weight = oracle.toUSD(token, totalScore); inside _computeServiceCommitmentExposureBps is a direct, uncapped interface call. The PR consistently replaces all other oracle calls with _safeToUSD (PaymentsBilling.sol, PaymentsDistribution.sol) but missed this one. If the oracle reverts or exceeds gas, proposeSlash will revert for any service with security commitments and a configured oracle.

🟠 MEDIUM Removed currentDeposits defensive clamp in delegation facet — src/facets/staking/StakingDelegationsFacet.sol

Line 206: _assetConfigs[assetHash].currentDeposits -= amountReturned; has the same clamp removal as DepositManager.sol. If the aggregate currentDeposits ever drifts below the amount being withdrawn, the transaction underflow-reverts and bricks the executeDelegatorUnstakeAndWithdraw path for that asset.

🟠 MEDIUM Removed currentDeposits defensive clamp risks withdrawal brick — src/staking/DepositManager.sol

Line 225: _assetConfigs[assetHash].currentDeposits -= readyAmounts[i]; replaced a saturating subtraction that clamped to zero when currentDeposits < readyAmounts[i]. The old code explicitly noted this was for 'upgrade safety (in case currentDeposits was previously incorrect).' In Solidity 0.8+, an underflow reverts the entire transaction, so any historical accounting drift will brick withdrawals for all users of that asset.

🟠 MEDIUM UPGRADER_ROLE change requires documented pre-upgrade step for live proxies — src/staking/MultiAssetDelegation.sol

Line 117: _authorizeUpgrade now requires UPGRADER_ROLE instead of ADMIN_ROLE. While initialize() at line 56 grants UPGRADER_ROLE to admin for new deployments, existing UUPS proxies that upgrade to this implementation will succeed but all subsequent upgrades will fail if the upgrader holds ADMIN_ROLE but not UPGRADER_ROLE. Live proxies do not re-run initialize().

🟡 LOW _safeToUSDView is dead code with misleading natspec — src/core/Base.sol

Lines 801-812: _safeToUSDView is introduced with natspec stating it is the 'View variant of _safeToUSD for the activation-time baseline path.' However, PaymentsDistribution._initSubscriptionBaseline calls _safeToUSD (non-view, event-emitting) instead. _safeToUSDView is never invoked anywhere in the codebase.

🟡 LOW tryTransferPayment can revert on malformed ERC20 returndata — src/libraries/PaymentLib.sol

Lines 291-297: after a successful low-level ERC20 transfer call, if ret.length > 0 but < 32 bytes, abi.decode(ret, (bool)) at line 297 will revert. This defeats the isolation purpose of tryTransferPayment — a single malformed token could still brick the distribution batch.

🟡 LOW Test does not assert PushTransferFailed event emission — test/tangle/PaymentEdgeCases.t.sol

Lines 333-362: test_Payment_TreasuryRejectsETH_FoldsToOperatorPool comments that 'a PushTransferFailed event fires so monitoring picks it up,' but the test never verifies the event with vm.expectEmit. If the emit were accidentally removed, the test would still pass.

🟡 LOW rampStakeDown does not actually reduce active stake — test/tangle/SubscriptionEscrowInvariant.t.sol

Lines 128-134: rampStakeDown calls staking.scheduleDelegatorUnstake, which only pushes a BondLessRequest into _unstakeRequests. The stake remains active and continues contributing to getCumStakeSeconds until executeDelegatorUnstake() is called. Because the handler never executes the unstake, the 'adversarial stake unwind' is a no-op for billing purposes and the invariant's coverage of stake-down scenarios is vacuous.

_{tangletools · 2026-05-16T23:06:25Z · trace}

tangletools

✅ Approved — 8 non-blocking findings — `040d435d`

Reviewed all 54 changed files, ran both modified test suites (22 unit tests + 3 invariant tests, all passing), and traced cross-contract call graphs. The PR is a solid audit remediation that correctly gas-caps manager hooks and oracle calls, adds best-effort payment transfer isolation, and removes dead storage. Three medium-severity gaps remain: the slashing path still uses an un-capped oracle cal

Full findings and scores: review summary

_{tangletools · 2026-05-16T23:06:25Z · trace}

`_accrueOperatorWeights` previously called `oracle.toUSD` live at every bill to USD-normalize each (operator, asset) commitment leg. An operator who could influence the oracle for a single committed asset (low-cap token on a manipulable Uniswap V3 pool, misconfigured Chainlink feed) could inflate their share of the (capped-at-nominal) bill at the expense of honest co-operators — the customer was safe via the nominal-rate cap, but the operator pool was distributionally exposed. Fix: pin the per-(serviceId, op, assetHash) USD-per-1e18-token conversion at activation/join, then reuse the snapshot on every subsequent bill. ## Storage - `TangleStorage._baselinePriceByOpAsset[serviceId][op][assetHash]` — 18-decimal USD-per-1e18-token snapshot. Captured exactly when the matching TWAP cursor is first seeded. - `__gap` decremented 40 → 39 to absorb the new mapping slot. ## Snapshot capture - `PaymentsDistribution._snapshotBaselinePrice` runs inside `_initSubscriptionBaseline` for every (op, asset) the activation enumerates (both the per-commitment path and the bond-asset fallback). - `ServicesLifecycle._snapshotJoinPrice` mirrors the same logic in `_finalizeJoin` so operators joining post-activation get pinned at their join price rather than activation price. - Both helpers no-op if a snapshot already exists for the triple, so a rejoiner whose cursor still holds pre-leave values reuses the original snapshot (a price ramp during the absence cannot game the rejoin). - Both use `_safeToUSDView` so a reverting / disabled oracle stores `1 ether` (identity scale) — the bill-time conversion treats identity as "raw token-second weighting for this leg", consistent with the M-1 economic graceful-degrade behavior. ## Bill-time conversion `_accrueOperatorWeights` now reads the snapshot from storage: ``` contribution = (opDeltaRaw * exposureBps) / BPS_DENOMINATOR; if (useOracle && contribution > 0) { uint256 snapshot = _baselinePriceByOpAsset[serviceId][op][assetHash]; if (snapshot != 0) { contribution = (contribution * snapshot) / 1 ether; } else { // legacy seeding without a snapshot — fall back to live oracle contribution = _safeToUSD(oracleAddr, token, contribution); } } ``` The post-activation oracle is no longer read on the hot path when a snapshot exists; the operator's weight contribution is therefore immune to oracle drift in either direction. ## Tests / follow-up A negative-tested fuzz invariant (price manipulator + bill ⇒ weight ratio remains bounded) is the next thing to land. Wiring that requires a controllable oracle handler on `SubscriptionEscrowInvariant`; will follow in its own commit. Follow-up to PR #144 + #145. Closes the last deferred audit finding from the 2026-05-16 batch.

… audit) The `_forwardStakerShare` ERC20 path previously did: 1. `safeTransfer(distributor, amount)` 2. `try distributor.distributeServiceFee(...) catch { emit refund event }` If step 2 reverted after step 1 succeeded, the tokens were stranded at the distributor with no protocol-level recovery path. The native path doesn't have this — `call{value: amount}` is rolled back by the EVM on revert — but ERC20 transfers are independent of try/catch. Fix: bundle the transfer + distribute into a single diamond self-call so a distributor revert unwinds both legs of the operation. The outer caller wraps the self-call in try/catch and refunds the share to escrow when the inner reverts, matching the native-path behavior. ## Surface - `TanglePaymentsDistributionFacet.forwardStakerShareAtomic` — new external entry point, gated by `msg.sender == address(this)`. Registered selector. - `ITanglePaymentsInternal.forwardStakerShareAtomic` — new interface entry. - `PaymentsDistribution._forwardStakerShareAtomic` — internal body: does the safeTransfer immediately followed by the distributor call. - `PaymentsDistribution._forwardStakerShare` — ERC20 branch rewritten to invoke the self-call via `try this.forwardStakerShareAtomic(...) catch { refund to escrow }`. The native path is unchanged — its `call{ value: amount }` semantics already rolled back cleanly on distributor revert. ## Reentrancy posture The outer `_distributeBill` holds the OZ `nonReentrant` guard. The new self-call lands at the diamond's fallback, delegatecalls into the distribution facet, and the guard's ERC-7201 namespaced slot is shared — so the inner function does NOT carry `nonReentrant` and gates on the `msg.sender == address(this)` check instead. Same pattern as `distributeBillWithKeeper`, `depositToEscrow`, etc. ## Tests - `test_ForwardStakerShareAtomic_RejectsExternalCallers` — verifies the self-call gate rejects EOAs and admin. End-to-end atomic-rollback semantics (reverting distributor mock + bill flow) is the next thing to add as an invariant on `SubscriptionEscrowInvariant`. Also makes `MockServiceFeeDistributor.distributeServiceFee` `virtual` so test-only `RevertingDistributor` can override. Follow-up to PR #144 + #145. Closes the new MEDIUM finding F-001 from the post-batch redteam audit. Remaining audit findings (F-003 VPM Ownable two-step, F-004 missing whenNotPaused on 4 functions, F-005/F-006 missing gas caps on Operator-status-registry and ServiceFeeDistributor calls, F-008 role-admin siloing) are LOW/INFO and will land in a separate hardening sweep PR.

…t + F-001 atomic staker forward (#145) * fix(audits): escrow rescue path for stuck refunds (Reentrancy M-2) `withdrawRemainingEscrow` had no fallback when the escrow token blocklisted the service owner or paused transfers post-deposit — the customer's funded escrow stayed locked even after termination, with no admin or owner override. Adds `withdrawRemainingEscrowTo(uint64 serviceId, address payable to)`: - Caller must still be the service owner (same auth as the parameterless variant). - Recipient is owner-chosen and arbitrary (non-zero), so a blocklisted owner can route around the token's restriction by sending to a fresh address. - Recipient is emitted on `EscrowRefunded`, so off-chain bookkeeping sees the alternate destination explicitly. The parameterless `withdrawRemainingEscrow(uint64)` keeps its old semantics (refund to `svc.owner`) for the common path; the internal `_withdrawRemainingEscrow` does the work. Tests: - `test_Subscription_WithdrawRemainingEscrowTo_RoutesToAlternateRecipient` — fresh address receives the full remainder; escrow is drained. - `test_Subscription_WithdrawRemainingEscrowTo_RejectsZeroAddress` — ZeroAddress reverts. - `test_Subscription_WithdrawRemainingEscrowTo_OnlyServiceOwner` — non-owner caller reverts with `NotServiceOwner`. Follow-up to PR #144's audit batch. The remaining deferred item is the Economic H-1 oracle weight snapshot at activation, which warrants its own focused PR. * fix(audits): snapshot oracle price at activation/join (Economic H-1) `_accrueOperatorWeights` previously called `oracle.toUSD` live at every bill to USD-normalize each (operator, asset) commitment leg. An operator who could influence the oracle for a single committed asset (low-cap token on a manipulable Uniswap V3 pool, misconfigured Chainlink feed) could inflate their share of the (capped-at-nominal) bill at the expense of honest co-operators — the customer was safe via the nominal-rate cap, but the operator pool was distributionally exposed. Fix: pin the per-(serviceId, op, assetHash) USD-per-1e18-token conversion at activation/join, then reuse the snapshot on every subsequent bill. ## Storage - `TangleStorage._baselinePriceByOpAsset[serviceId][op][assetHash]` — 18-decimal USD-per-1e18-token snapshot. Captured exactly when the matching TWAP cursor is first seeded. - `__gap` decremented 40 → 39 to absorb the new mapping slot. ## Snapshot capture - `PaymentsDistribution._snapshotBaselinePrice` runs inside `_initSubscriptionBaseline` for every (op, asset) the activation enumerates (both the per-commitment path and the bond-asset fallback). - `ServicesLifecycle._snapshotJoinPrice` mirrors the same logic in `_finalizeJoin` so operators joining post-activation get pinned at their join price rather than activation price. - Both helpers no-op if a snapshot already exists for the triple, so a rejoiner whose cursor still holds pre-leave values reuses the original snapshot (a price ramp during the absence cannot game the rejoin). - Both use `_safeToUSDView` so a reverting / disabled oracle stores `1 ether` (identity scale) — the bill-time conversion treats identity as "raw token-second weighting for this leg", consistent with the M-1 economic graceful-degrade behavior. ## Bill-time conversion `_accrueOperatorWeights` now reads the snapshot from storage: ``` contribution = (opDeltaRaw * exposureBps) / BPS_DENOMINATOR; if (useOracle && contribution > 0) { uint256 snapshot = _baselinePriceByOpAsset[serviceId][op][assetHash]; if (snapshot != 0) { contribution = (contribution * snapshot) / 1 ether; } else { // legacy seeding without a snapshot — fall back to live oracle contribution = _safeToUSD(oracleAddr, token, contribution); } } ``` The post-activation oracle is no longer read on the hot path when a snapshot exists; the operator's weight contribution is therefore immune to oracle drift in either direction. ## Tests / follow-up A negative-tested fuzz invariant (price manipulator + bill ⇒ weight ratio remains bounded) is the next thing to land. Wiring that requires a controllable oracle handler on `SubscriptionEscrowInvariant`; will follow in its own commit. Follow-up to PR #144 + #145. Closes the last deferred audit finding from the 2026-05-16 batch. * fix(audits): atomic ERC20 staker-share forward (F-001 from post-batch audit) The `_forwardStakerShare` ERC20 path previously did: 1. `safeTransfer(distributor, amount)` 2. `try distributor.distributeServiceFee(...) catch { emit refund event }` If step 2 reverted after step 1 succeeded, the tokens were stranded at the distributor with no protocol-level recovery path. The native path doesn't have this — `call{value: amount}` is rolled back by the EVM on revert — but ERC20 transfers are independent of try/catch. Fix: bundle the transfer + distribute into a single diamond self-call so a distributor revert unwinds both legs of the operation. The outer caller wraps the self-call in try/catch and refunds the share to escrow when the inner reverts, matching the native-path behavior. ## Surface - `TanglePaymentsDistributionFacet.forwardStakerShareAtomic` — new external entry point, gated by `msg.sender == address(this)`. Registered selector. - `ITanglePaymentsInternal.forwardStakerShareAtomic` — new interface entry. - `PaymentsDistribution._forwardStakerShareAtomic` — internal body: does the safeTransfer immediately followed by the distributor call. - `PaymentsDistribution._forwardStakerShare` — ERC20 branch rewritten to invoke the self-call via `try this.forwardStakerShareAtomic(...) catch { refund to escrow }`. The native path is unchanged — its `call{ value: amount }` semantics already rolled back cleanly on distributor revert. ## Reentrancy posture The outer `_distributeBill` holds the OZ `nonReentrant` guard. The new self-call lands at the diamond's fallback, delegatecalls into the distribution facet, and the guard's ERC-7201 namespaced slot is shared — so the inner function does NOT carry `nonReentrant` and gates on the `msg.sender == address(this)` check instead. Same pattern as `distributeBillWithKeeper`, `depositToEscrow`, etc. ## Tests - `test_ForwardStakerShareAtomic_RejectsExternalCallers` — verifies the self-call gate rejects EOAs and admin. End-to-end atomic-rollback semantics (reverting distributor mock + bill flow) is the next thing to add as an invariant on `SubscriptionEscrowInvariant`. Also makes `MockServiceFeeDistributor.distributeServiceFee` `virtual` so test-only `RevertingDistributor` can override. Follow-up to PR #144 + #145. Closes the new MEDIUM finding F-001 from the post-batch redteam audit. Remaining audit findings (F-003 VPM Ownable two-step, F-004 missing whenNotPaused on 4 functions, F-005/F-006 missing gas caps on Operator-status-registry and ServiceFeeDistributor calls, F-008 role-admin siloing) are LOW/INFO and will land in a separate hardening sweep PR.

… + bindings v0.17.1 (#147) * fix(audits): escrow rescue path for stuck refunds (Reentrancy M-2) `withdrawRemainingEscrow` had no fallback when the escrow token blocklisted the service owner or paused transfers post-deposit — the customer's funded escrow stayed locked even after termination, with no admin or owner override. Adds `withdrawRemainingEscrowTo(uint64 serviceId, address payable to)`: - Caller must still be the service owner (same auth as the parameterless variant). - Recipient is owner-chosen and arbitrary (non-zero), so a blocklisted owner can route around the token's restriction by sending to a fresh address. - Recipient is emitted on `EscrowRefunded`, so off-chain bookkeeping sees the alternate destination explicitly. The parameterless `withdrawRemainingEscrow(uint64)` keeps its old semantics (refund to `svc.owner`) for the common path; the internal `_withdrawRemainingEscrow` does the work. Tests: - `test_Subscription_WithdrawRemainingEscrowTo_RoutesToAlternateRecipient` — fresh address receives the full remainder; escrow is drained. - `test_Subscription_WithdrawRemainingEscrowTo_RejectsZeroAddress` — ZeroAddress reverts. - `test_Subscription_WithdrawRemainingEscrowTo_OnlyServiceOwner` — non-owner caller reverts with `NotServiceOwner`. Follow-up to PR #144's audit batch. The remaining deferred item is the Economic H-1 oracle weight snapshot at activation, which warrants its own focused PR. * fix(audits): snapshot oracle price at activation/join (Economic H-1) `_accrueOperatorWeights` previously called `oracle.toUSD` live at every bill to USD-normalize each (operator, asset) commitment leg. An operator who could influence the oracle for a single committed asset (low-cap token on a manipulable Uniswap V3 pool, misconfigured Chainlink feed) could inflate their share of the (capped-at-nominal) bill at the expense of honest co-operators — the customer was safe via the nominal-rate cap, but the operator pool was distributionally exposed. Fix: pin the per-(serviceId, op, assetHash) USD-per-1e18-token conversion at activation/join, then reuse the snapshot on every subsequent bill. ## Storage - `TangleStorage._baselinePriceByOpAsset[serviceId][op][assetHash]` — 18-decimal USD-per-1e18-token snapshot. Captured exactly when the matching TWAP cursor is first seeded. - `__gap` decremented 40 → 39 to absorb the new mapping slot. ## Snapshot capture - `PaymentsDistribution._snapshotBaselinePrice` runs inside `_initSubscriptionBaseline` for every (op, asset) the activation enumerates (both the per-commitment path and the bond-asset fallback). - `ServicesLifecycle._snapshotJoinPrice` mirrors the same logic in `_finalizeJoin` so operators joining post-activation get pinned at their join price rather than activation price. - Both helpers no-op if a snapshot already exists for the triple, so a rejoiner whose cursor still holds pre-leave values reuses the original snapshot (a price ramp during the absence cannot game the rejoin). - Both use `_safeToUSDView` so a reverting / disabled oracle stores `1 ether` (identity scale) — the bill-time conversion treats identity as "raw token-second weighting for this leg", consistent with the M-1 economic graceful-degrade behavior. ## Bill-time conversion `_accrueOperatorWeights` now reads the snapshot from storage: ``` contribution = (opDeltaRaw * exposureBps) / BPS_DENOMINATOR; if (useOracle && contribution > 0) { uint256 snapshot = _baselinePriceByOpAsset[serviceId][op][assetHash]; if (snapshot != 0) { contribution = (contribution * snapshot) / 1 ether; } else { // legacy seeding without a snapshot — fall back to live oracle contribution = _safeToUSD(oracleAddr, token, contribution); } } ``` The post-activation oracle is no longer read on the hot path when a snapshot exists; the operator's weight contribution is therefore immune to oracle drift in either direction. ## Tests / follow-up A negative-tested fuzz invariant (price manipulator + bill ⇒ weight ratio remains bounded) is the next thing to land. Wiring that requires a controllable oracle handler on `SubscriptionEscrowInvariant`; will follow in its own commit. Follow-up to PR #144 + #145. Closes the last deferred audit finding from the 2026-05-16 batch. * fix(audits): atomic ERC20 staker-share forward (F-001 from post-batch audit) The `_forwardStakerShare` ERC20 path previously did: 1. `safeTransfer(distributor, amount)` 2. `try distributor.distributeServiceFee(...) catch { emit refund event }` If step 2 reverted after step 1 succeeded, the tokens were stranded at the distributor with no protocol-level recovery path. The native path doesn't have this — `call{value: amount}` is rolled back by the EVM on revert — but ERC20 transfers are independent of try/catch. Fix: bundle the transfer + distribute into a single diamond self-call so a distributor revert unwinds both legs of the operation. The outer caller wraps the self-call in try/catch and refunds the share to escrow when the inner reverts, matching the native-path behavior. ## Surface - `TanglePaymentsDistributionFacet.forwardStakerShareAtomic` — new external entry point, gated by `msg.sender == address(this)`. Registered selector. - `ITanglePaymentsInternal.forwardStakerShareAtomic` — new interface entry. - `PaymentsDistribution._forwardStakerShareAtomic` — internal body: does the safeTransfer immediately followed by the distributor call. - `PaymentsDistribution._forwardStakerShare` — ERC20 branch rewritten to invoke the self-call via `try this.forwardStakerShareAtomic(...) catch { refund to escrow }`. The native path is unchanged — its `call{ value: amount }` semantics already rolled back cleanly on distributor revert. ## Reentrancy posture The outer `_distributeBill` holds the OZ `nonReentrant` guard. The new self-call lands at the diamond's fallback, delegatecalls into the distribution facet, and the guard's ERC-7201 namespaced slot is shared — so the inner function does NOT carry `nonReentrant` and gates on the `msg.sender == address(this)` check instead. Same pattern as `distributeBillWithKeeper`, `depositToEscrow`, etc. ## Tests - `test_ForwardStakerShareAtomic_RejectsExternalCallers` — verifies the self-call gate rejects EOAs and admin. End-to-end atomic-rollback semantics (reverting distributor mock + bill flow) is the next thing to add as an invariant on `SubscriptionEscrowInvariant`. Also makes `MockServiceFeeDistributor.distributeServiceFee` `virtual` so test-only `RevertingDistributor` can override. Follow-up to PR #144 + #145. Closes the new MEDIUM finding F-001 from the post-batch redteam audit. Remaining audit findings (F-003 VPM Ownable two-step, F-004 missing whenNotPaused on 4 functions, F-005/F-006 missing gas caps on Operator-status-registry and ServiceFeeDistributor calls, F-008 role-admin siloing) are LOW/INFO and will land in a separate hardening sweep PR. * chore(audits): polish — configurable operator cap, pull-payment staker forward, audit-tag sweep Three professional cleanups, net code REMOVAL (-203 / +151 across 50 files). ## 1. Operator cap is now governance-tunable, not hardcoded `ProtocolConfig.MAX_OPERATORS_PER_SERVICE = 64` is replaced by a governance- tunable storage variable `_maxOperatorsPerService`, default 256, with an admin setter `setMaxOperatorsPerService(uint32)`. The constant is renamed `DEFAULT_MAX_OPERATORS_PER_SERVICE` and is referenced only at protocol init. A cloud-platform-scale service can now register more operators per service than the arbitrary 64-cap previously imposed. The bound is still real (loops in bill/distribute/terminate paths must terminate), but it is no longer hardcoded — admin can raise or lower it through normal governance flow. Reads at the two enforcement sites (`ServicesRequests._validateOperatorBounds`, `ServicesLifecycle` join path) switch from compile-time constant to storage. ## 2. F-001 ERC20 staker-share refactored to pull-payment The earlier atomic-wrapper approach (`forwardStakerShareAtomic` self-call, new selector on `TanglePaymentsDistributionFacet`, new entry on `ITanglePaymentsInternal`) is replaced with the standard pull-payment pattern, deleting all that scaffolding. `ServiceFeeDistributor.distributeServiceFee` now pulls ERC20 via `safeTransferFrom(msg.sender, ...)` instead of expecting the caller to push first. `PaymentsDistribution._forwardStakerShare` ERC20 branch becomes: ``` IERC20(token).forceApprove(distributor, amount); try distributor.distributeServiceFee(...) { IERC20(token).forceApprove(distributor, 0); // defensive clear } catch (bytes memory reason) { IERC20(token).forceApprove(distributor, 0); // revoke unused approval _refundStakerShareToEscrow(...); // share back to escrow } ``` If the distributor reverts, no tokens leave the diamond — no stranding, no recovery path needed. The defensive `forceApprove(distributor, 0)` after a successful call clears any under-pull residue. Net: deletes `forwardStakerShareAtomic` from `TanglePaymentsDistributionFacet` + `ITanglePaymentsInternal`, deletes `_forwardStakerShareAtomic` helper, deletes the corresponding test mock interface, simplifies the call site. ## 3. Audit-tag sweep, take two The earlier sweep on `main` missed several patterns and didn't cover `test/`. This one closes the gap: - `Round N audit X-N`, `(Round N)`, `Round N follow-up` prefixes - `H-1 economic surface from the 2026-05-16 audit` (a phrasing I introduced myself in the H-1 fix — caught) - `M-N FIX:` / `H-N FIX:` / `C-N FIX:` patterns across `test/` - Manual fix for `test/beacon/LiveBeaconTest.t.sol` where the previous sweep collapsed a NatSpec block across a function declaration Source comments now describe current behavior. Historical narrative belongs in commit messages and PR descriptions, not in code. ## Test plan - [x] `forge build` clean - [x] `forge test --no-match-path test/Integration.t.sol --no-match-test invariant` green (running) Stacked on #145 which contains M-2 + H-1 + F-001 (atomic-wrapper version). This PR rebases F-001 onto the pull-payment shape, picks up the operator-cap change, and finishes the audit-tag cleanup. * chore(bindings): release v0.17.1 Regenerate against polish branch (commit 50713b8). Surfaces from this audit batch + polish: - New: withdrawRemainingEscrowTo (M-2 escrow rescue) - New: setMaxOperatorsPerService + maxOperatorsPerService view + MaxOperatorsPerServiceUpdated event (governance-tunable operator cap) - New: PushTransferFailed, PriceOracleFallback events (M-1 reentrancy + M-1 economic graceful-degrade) - New: rescuePendingDisputeBond admin path (slashing L-1 follow-up; if applicable) - Behavioral: ServiceFeeDistributor.distributeServiceFee now pulls ERC20 via transferFrom (F-001 pull-payment) - Storage: _baselinePriceByOpAsset mapping (H-1 oracle price snapshot at activation) - Removed: forwardStakerShareAtomic (superseded by pull-payment)

drewstone added 6 commits May 16, 2026 14:55

audit: storage + UUPS upgrade red-team 2026-05-16

c7ef163

drewstone mentioned this pull request May 16, 2026

fix(audits): follow-up batch — M-2 escrow rescue + H-1 oracle snapshot + F-001 atomic staker forward #145

Merged

tangletools approved these changes May 16, 2026

View reviewed changes

drewstone merged commit 90dcf64 into main May 17, 2026
1 of 2 checks passed

drewstone mentioned this pull request May 17, 2026

chore(v0.17.1): land stranded M-2 + H-1 + F-001 + operator-cap polish + bindings v0.17.1 #147

Merged

3 tasks

tangletools mentioned this pull request May 20, 2026

chore(deploy): base-sepolia readiness — 2-phase bootstrap + config cleanup #150

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(v0.17.1): audit batch — reports + stress harness + remediation#144

chore(v0.17.1): audit batch — reports + stress harness + remediation#144
drewstone merged 6 commits into
mainfrom
chore/v0.17.1-audit-batch

drewstone commented May 16, 2026

Uh oh!

tangletools commented May 16, 2026 •

edited

Loading

Uh oh!

tangletools left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

drewstone commented May 16, 2026

Audit reports landed (docs/audits/REDTEAM-*-2026-05-16.md)

Stress harness landed

Remediations in this PR

HIGH — DoS H-1

HIGH — DoS H-2 (also closes Reentrancy M-3)

MEDIUM — Reentrancy M-1

MEDIUM — Economic M-1

LOW — Storage L-1

Greenfield cleanup

Fuzz coverage

What's deliberately deferred

Test plan

Uh oh!

tangletools commented May 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ No Blockers — 040d435d

Uh oh!

tangletools left a comment

Choose a reason for hiding this comment

✅ Approved — 8 non-blocking findings — 040d435d

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Audit reports landed (`docs/audits/REDTEAM-*-2026-05-16.md`)

tangletools commented May 16, 2026 •

edited

Loading

✅ No Blockers — `040d435d`

✅ Approved — 8 non-blocking findings — `040d435d`