Guardian's review of DollarStore for DollarStore, published October 2026. The report records 11 findings across 2 review rounds, including 4 low and 7 informational.
- Published
- Review window
- October 13 to 26, 2026
- Rounds
- Main Review, Remediation Review
- Language
- Solidity
- 0 Critical
- 0 High
- 0 Medium
- 4 Low
- 7 Informational
Scope
19 files in scope · 1,752 nSLOC
| File | nSLOC | Lines |
|---|---|---|
foundry.toml | 37 | 48 |
script/CreateSpoke.s.sol | 78 | 113 |
script/Deploy.s.sol | 32 | 57 |
script/DeployGovernance.s.sol | 83 | 156 |
script/Upgrade.s.sol | 17 | 42 |
src/DLRS.sol | 35 | 68 |
src/DollarStore.sol | 515 | 898 |
src/interfaces/AggregatorV3Interface.sol | 3 | 7 |
src/interfaces/IDollarStore.sol | 20 | 52 |
src/libraries/NormalizationLib.sol | 18 | 52 |
src/libraries/PegLib.sol | 57 | 94 |
src/libraries/QueueLib.sol | 105 | 152 |
src/libraries/SpokeAdminLib.sol | 75 | 118 |
src/libraries/SpokeLifecycleLib.sol | 201 | 290 |
src/libraries/SpokeShareLib.sol | 23 | 60 |
src/libraries/SwapRouteLib.sol | 339 | 487 |
src/storage/CoreStorage.sol | 23 | 58 |
src/storage/QueueStorage.sol | 45 | 127 |
src/storage/RegistryStorage.sol | 46 | 105 |
Findings 11
Main Review
7 findings · October 13 to 19, 2026-
L-01 Low canInflow panics on a malformed feed instead of returning false Unexpected Behavior Resolved
Description
PegLib.canInflow is documented as non-reverting ("a reverting feed reads as false") and exists so callers that must SKIP rather than revert on an unhealthy asset can do so: SwapRouteLib.triggerSpokeQueues calls it to skip a deposit-paused/off-peg offer so an unrelated bad asset cannot DoS a legitimate deposit, and SwapRouteLib.quote uses it to return 0.
The try/catch in canInflow only wraps the external latestRoundData() call. The staleness subtraction
block.timestamp - updatedAtand the scaling10 ** (PRICE_DECIMALS - uint256(agg.decimals()))execute inside the try SUCCESS block, so a revert from them is NOT caught bycatch { return false; }. A feed that reports a futureupdatedAt(updatedAt > block.timestamp) therefore underflows and panics (Panic 0x11) instead of returning false. checkPeg repeats the sameblock.timestamp - updatedAtunderflow.Impact: for the affected asset, canInflow reverts rather than skipping, so triggerSpokeQueues (and any caller relying on the non-reverting form) reverts the whole call. A legit spoke deposit that would otherwise settle queues while skipping the bad asset instead reverts, i.e. the exact DoS the non-reverting API was written to avoid.
The blast radius is bounded to the asset with the malformed feed and can be contained by a pauser pausing that asset/pool, which is why this is low severity rather than higher
Recommendation
Either bound and validate the feed values before the arithmetic (reject updatedAt > block.timestamp by returning false), or move the subtraction / scaling into a helper that can revert and be invoked through try / catch so the failure is converted to false.
Apply the same guard to checkPeg's staleness subtraction so a future updatedAt reverts with a clear error rather than a bare panic
Resolution
DollarStore Team: Fixed in 1b3aae7, wider than recommended. canInflow returns false for a future updatedAt, a reverting decimals(), more than 18 feed decimals and an answer whose scaling would overflow. It also reads the feed with a raw staticcall, so a response that is too short, has out-of-range uint80 or uint8 values, or comes from an address with no code reads as unhealthy instead of reverting. Each feed call, in canInflow and checkPeg, may use at most 100,000 gas, so a feed that never answers fails the check instead of using up the caller's gas; a standard Chainlink read measured about 16,000. As recommended, checkPeg reverts PriceStale for a future updatedAt. A fuzz test checks that, for any feed response, a queue offering that asset is settled exactly when a deposit of it succeeds, so canInflow and checkPeg agree.
-
L-02 Low A scheduled governor rescue run mid-swap sweeps pulled-but-unbooked escrow and leaves the queue position unbacked Reentrancy Resolved
Description
DollarStore.rescueTokensisonlyGovernorbut notnonReentrant. It is the only token-moving entry point in the contract without the guard (swap, swapExactInput, swapSpokeToSpoke, deposit, withdraw, cancelQueue, processQueue, and even the non-token-movinghaircutEscrowall carry it).foundry.tomleven documents the intent, excludingreentrancy-eventsbecause "every function that moves tokens is nonReentrant",rescueTokensbreaks that ruleSwapRouteLib.swap performs the following order:
1 - pullExact(offerAsset, nativePulled) pulls the offer into the contract
2 - fillDirected()` fills from the opposite queue / reserves
3 - IERC20(wantAsset).safeTransfer(msg.sender) pays the caller the filled
wantAsset4 - enqueueRemainder() only now books the unfilled remainder as queue escrow
Step 3 is an external transfer to the attacker (the swapper) that happens before step 4 records the escrow. A listed asset whose transfer invokes a hook (the repo's own
MockReentrantERC20models exactly this, and every other token-moving path is guarded against it) executes attacker code here.rescueTokenscomputesexcess = balanceOf(this) - (escrow + reserves)and transfers it out. At the reentrancy point the pulled remainder is present in the balance but is not yet intotalEscrowedByAsset, so it is counted as excess and swept to the rescue recipient.swapthen records the queue position, which is now unbacked:balance < reserves + escrowby exactly the remainder.The rescue is a privileged/governor action, but the governor timelock is intentionally configured with an open executor (
executors = [address(0)], so any unprivileged account can execute an already-scheduled, readyrescueTokensat the moment of its choosing, including from inside the swap's outbound-transfer callback. The attacker does not need to control the rescue'stoaddress; the harm is that a legitimate scheduled rescue over-sweeps.This violates the documented guarantee: "rescueTokens sweeps only balance above what the books account for, and queue escrow is always accounted for" and "no role can take escrow or reserves out of the contract"
Impact: with the escrow aggregate short, a later honest claimant can be parked/unpaid once earlier payouts consume the surviving balance.
Preconditions
A listed asset whose transfer calls back (hook/ERC777-style). The protocol treats such tokens as in scope (it ships MockReentrantERC20 and guards every other token-moving path), and listing enforces only a receiver probe, not absence of hooks
A governor
rescueTokensoperation that is scheduled and past its timelock delay (ordinary governance activity)Recommendation
Add
nonReentranttoDollarStore.rescueTokens, mirroringhaircutEscrowResolution
DollarStore Team: Fixed in ab78b9c, as recommended. rescueTokens is nonReentrant, so a rescue executed from a token callback during a swap reverts. The test uses a real TimelockController with an open executor and a token that executes the ready rescue from its payout callback; the swap reverts. A second test checks that the same rescue, run after the swap, sweeps only the excess and leaves reserves and escrow fully backed.
Separately, SECURITY.md and DEPLOY.md now state as a listing rule that tokens with transfer hooks are not listed (a50bab4). The guard stays in place either way.
-
L-03 Low Stale pool ID in capped-listing batches leaves a new spoke uncapped while overwriting another pool's cap Validation Resolved
Description
The script schedules two actions together: "create the new pool" and "cap it", guessing the new pool's future ID number from today's pool count. That guess is frozen into the payload at schedule time:
// script/CreateSpoke.s.sol:55,66 ( id baked in at schedule time ) expectedPoolId = uint16(DollarStore(store).poolCount()); // read NOW payloads[1] = abi.encodeCall(DollarStore.setLaunchCap, (expectedPoolId, launchCap)); // id frozenThat guess is not tied to the pool the batch actually creates. The real ID is assigned only when the transaction runs, based on how many pools exist then:
// src/libraries/SpokeAdminLib.sol:57-63 ( real id assigned at EXECUTION time ) uint256 newIndex = r.pools.length; // grows if any other createSpoke lands first poolId = uint16(newIndex); // != expectedPoolId -> wrong pool cappedSo if someone else creates a different pool in between, the new pool lands on a different number, the cap is applied to the wrong (older) pool, and the new one launches uncapped.
The safety check inside
execute()is fake: it compares the pool count to itself in the same moment, so it always passes// script/CreateSpoke.s.sol:107-110 require( targets.length == 1 || uint16(DollarStore(store).poolCount()) == expectedPoolId, // both sides = poolCount() "poolCount moved since scheduling: re-schedule" );The script itself is safe-ish (rebuilding the batch yields a new hash, so the timelock says "not ready"), but the batch already scheduled on-chain stays valid and anyone can run it, since execution is open
executors = [address(0)]Result: exposure limits get silently messed up: one pool can accept more deposits than intended, another pool's cap is changed unexpectedly
Impact is bounded and operational; it just breaks risk limits until governance fixes the caps
And needs two listings overlapping in time plus someone executing the stale batch
Recommendation
Bind creation and its launch cap in one on chain operation using the id actually created, preferably by adding a cap argument to createSpoke (or having it return and atomically apply the cap) so a stale predicted id cannot exist
Resolution
DollarStore Team: Fixed in bbb13a4, as recommended. createSpoke takes the launch cap as a fourth argument and sets it on the pool it creates, in the same call. CreateSpoke.s.sol queues that one call and no longer predicts a pool id.
-
L-04 Low A ready governor syncReserves run from a refund or payout callback writes reserves down by the in-flight amount a second time Reentrancy Resolved
Description
This uses the same vector and preconditions as M-01 (a ready governor operation executed through the open timelock executor from a hook-token callback), but on
syncReservesinstead ofrescueTokens, in different windows (a cancel refund and a settlement payout), and with a different effect: reserves are written down below the real balance instead of escrow being swept.DollarStore.syncReservesisonlyGovernorbut notnonReentrant. Its body sets the reserve from the live token balance:// src/libraries/SpokeAdminLib.sol:92-100 (excerpt) uint256 actualUnits = IERC20(asset).balanceOf(address(this)) / cfg.scalingFactor; uint256 escrowUnits = QueueStorage.layout().totalEscrowedByAsset[asset]; uint256 prevReserves = r.reserves[poolId][asset]; if (actualUnits >= prevReserves + escrowUnits) revert IDollarStore.ReservesNotDrifted(asset); uint256 newReserves = actualUnits - escrowUnits; r.reserves[poolId][asset] = newReserves;Two paths pay tokens out before they update the books:
1 -
SwapRouteLib.cancelPositionrefunds the owner, and only thenQueueLib.removelowers escrow2 -
SwapRouteLib.settleSameDirectionpays the owner, and only thenapplyReserveFilllowers reserves (reached fromprocessQueue, inline settlement in swaps, and the spoke-deposit trigger)// src/libraries/SwapRouteLib.sol:259-271 (cancelPosition) /// Safe because the wrapper is nonReentrant, so the token call cannot re-enter a mutator. ... if (tryTransfer(offerAsset, owner_, NormalizationLib.toNative(amount, scaling))) { QueueLib.remove(qs, positionId);The code relies on the guard to make this ordering safe ("the token call cannot re-enter a mutator"), but
syncReservesis a mutator the guard does not cover, and it derives its write from the very balance the in-flight transfer has just changed.The governor timelock is configured with an open executor (
executors[0] = address(0), script/DeployGovernance.s.sol:84), so any unprivileged account can execute a readysyncReservesoperation at the moment of its choosing, including from inside the refund or payout callback of its own position.At that moment the balance is already down by the in-flight amount A while the books still hold it, so the drift check passes on the transfer alone and reserves are written to
balance - escrow. The outer path then lowers escrow (cancel) or reserves (settle) by A again:-
Cancel window: reserves end at R - L - A against a balance of R - L, where R is the booked reserve and L the real loss. A tokens stay in the contract outside the books, and since
syncReservesonly ever lowers reserves, no function books them back. -
Settle window: the same payout is subtracted twice, and reserves end at R - L - 2A against a balance of R - L - A.
No real loss is needed. A ready operation whose normal execution reverts
ReservesNotDriftedis not consumed by that revert, and OpenZeppelin timelock operations never expire, so it stays ready and succeeds inside the window, writing reserves to R - A with zero loss.Example: R = 1,000,000, L = 50,000, A = 200,000. The honest sync writes 950,000. Run from the cancel refund callback it writes 750,000, and a holder's withdrawal of 950,000 that succeeds without the attack reverts
InsufficientReserves(asset, 950_000e6, 750_000e6). With A = R - L the reserve is written to 0.This violates the documented guarantee: "
syncReservesonly ever decreases reserves, to true them up to the real balance after an external loss" (SECURITY.md, Reserve accounting) and "Sync recorded reserves down to the actual balance minus queue escrow" (IDollarStore.syncReserves).Impact: up to the whole booked reserve of the targeted asset stops backing DLRS (hub asset) or the spoke's LP shares (spoke asset) while the tokens stay in the contract, redemptions revert
InsufficientReservesonce the gap is reached, and the governor's write-down operation is consumed.Preconditions: a listed asset whose transfer calls back the recipient (hook/ERC777-style), a class the protocol treats as in scope (it ships
MockReentrantERC20, guards every user-facing token-moving path, justifies the cancel path's ordering in code by the reentrancy guard, and its listing preconditions in SECURITY.md and DEPLOY.md exclude only sender-only freezes and tokens that reject zero-value transfers, not tokens with transfer hooks); and a governorsyncReservesoperation that is scheduled and past its timelock delay, which is ordinary loss-reconciliation activity.Recommendation
Add
nonReentranttoDollarStore.syncReserves, and apply the same guard to any other governor function that derives a write from the live token balance or from pool state that a user action changes mid-flight (for exampleremovePool), so no such operation can execute while a payout or refund is in progress. Timelock batches are unaffected because each call in a batch enters and leaves the guard separately.Resolution
DollarStore Team: Fixed in 58e9e9a, as recommended. syncReserves is nonReentrant, so a sync executed from a cancel refund or a queue payout callback reverts. If the token passes that revert on, the transfer fails and the position is parked with its escrow; if it swallows it, nothing was synced. As suggested, addHubAsset, createSpoke, windDownSpoke and removePool are nonReentrant too, so no governor action that changes pool or asset state runs inside a token transfer. Timelock batches are unaffected.
Separately, SECURITY.md and DEPLOY.md now state as a listing rule that tokens with transfer hooks are not listed (a50bab4). The guards stay in place either way.
-
-
I-01 Informational Security NatSpec names the guardian as canceller, the governor as upgrade authority and the guardian as deposit pauser, contradicting the shipped role model Documentation Resolved
Description
The NatSpec on the core contract, its interface and its storage still describes milestone-era role assignments that the shipped code and SECURITY.md contradict. The 3-day
MAX_STALENESSceiling is justified as harmless because a wrong governor action is "cancellable by the guardian". However, the guardian holds no canceller role: SECURITY.md:66 says "The guardian is not a canceller", DeployGovernance.s.sol:29 says "The guardian holds no cancel power", and on mainnetcheckSeparationrequires the guardian to differ from the veto Safe and both proposer Safes so that it holds none. The contract notice calls upgrades "governor-gated", but_authorizeUpgradeisonlyUpgrader(DollarStore.sol:988), a separate role behind a 14-day timelock.AssetConfig.depositPausedis documented as "set by guardian", whilepauseDepositsisonlyPauser(DollarStore.sol:847) andunpauseDepositsisonlyUnpauser(:855).IDollarStore.deposit, the interface integrators compile against, is documented as "Hub-only in this milestone", returning "DLRS minted", and that notice is inherited into the implementation through@inheritdoc. In fact, spoke deposits return receipt shares and emit onlySpokeLiquidityAdded(DollarStore.sol:584, 617), andNotEnabledis never thrown.// src/DollarStore.sol:28-29 /// @notice Establishes the upgradeable core: ERC-7201 namespaced storage, two-step /// governor/guardian roles, pause/unpause, and governor-gated UUPS upgrades. // src/DollarStore.sol:54-57 /// a ceiling below what a listed feed needs bricks swaps and takes an upgrade to lift, while a /// generous one only matters if a governor action is wrong, which is timelocked and cancellable /// by the guardian. uint256 private constant MAX_STALENESS = 3 days; // src/storage/RegistryStorage.sol:41 /// @notice Per-asset deposit pause (set by guardian in M5). // src/interfaces/IDollarStore.sol:266-268 /// @notice Deposit `amount` (native units) of a hub asset and receive DLRS 1:1 in normalized /// units. Sub-unit dust stays with the caller. Hub-only in this milestone. /// @return receiptUnits The normalized 6dp amount credited (DLRS minted).These comments are what block explorers, integrators and incident responders read, and each one points at the wrong actor for a safety control:
- A reader of the staleness rationale assumes the guardian can veto a mistaken
setMaxStaleness, when only the veto Safe or the proposing Safe can cancel it. - A reader of the contract notice watches the governor timelock for upgrades instead of the upgrader timelock.
- An operator following the storage docs during an incident sends a per-asset pause to the guardian Safe, and the call reverts
OnlyPauser. - An integrator building against
IDollarStoretreats a spoke deposit's return value as DLRS units, when it is a share amount whose value diverges from units after any write-down.
Recommendation
Rewrite these comments to match the deployed role model:
- The
MAX_STALENESSrationale should name the veto Safe as the canceller. - The contract notice should name the upgrader as upgrade authority and the pausers and unpauser as the pause roles.
depositPausedshould name the pauser and unpauser.IDollarStore.depositshould document both the hub path (DLRS minted) and the spoke path (receipt shares,SpokeLiquidityAdded).
Also remove the unused
NotEnablederror and the "Milestone M1" headers onIDollarStoreandDollarStore.Resolution
DollarStore Team: Fixed in 3f6e005, as recommended. The NatSpec names the veto Safe as canceller, the upgrader as upgrade authority, and the pausers and unpauser as the pause roles, and documents both deposit paths (DLRS minted on the hub; receipt shares and SpokeLiquidityAdded on a spoke). The NotEnabled error and the milestone labels are removed.
- A reader of the staleness rationale assumes the guardian can veto a mistaken
-
I-02 Informational Relisting a removed asset silently clears a pauser's deposit pause and the probe exemption, bypassing the unpauser Access Control Resolved
Description
removePoolunlists the spoke asset by flipping onlylistedto false (SpokeLifecycleLib.sol:79), so the asset'sdepositPausedandprobeExemptflags survive. Once the asset is unlisted,pauseDeposits,unpauseDepositsandsetProbeExemptall revertAssetNotListed, so no role can inspect or carry those flags across the relist. Relisting, which PROTOCOL.md "Relisting" and SECURITY.md "Fully impaired spoke recovery" describe as the recovery path, goes throughaddHubAssetorcreateSpoke. Both overwrite the wholeAssetConfigwith both flags set to false, and both emit onlyAssetListedorPoolCreated.// src/libraries/SpokeLifecycleLib.sol:79 (removePool) r.assetConfig[sa].listed = false; // depositPaused and probeExempt are left as they were // src/DollarStore.sol:330-338 (addHubAsset); src/libraries/SpokeAdminLib.sol:69-77 (createSpoke) is identical r.assetConfig[asset] = RegistryStorage.AssetConfig({ poolId: 0, decimals: dec, scalingFactor: sf, priceFeed: priceFeed, listed: true, depositPaused: false, // a pauser's deposit pause is lifted here probeExempt: false // the governor's exemption is dropped here });Suppose a pauser paused deposits of an asset during an incident and the asset is later retired and relisted. Deposits and swaps offering it reopen the moment anyone executes the relist through the open governor executor, with no decision by the unpauser. That breaks the role split SECURITY.md:83-106 and :197 sets out ("Pausing and lifting a pause are separate roles", "The unpauser lifts the pause. No pauser can"), and the pauser can only re-pause after inflows have resumed. Dropping
probeExemptbrings back the zero-value issuer probe on exits. For a token that rejects zero-value transfers, which is the case the exemption exists for, exits that pay none of that asset revertExitBlockedByIssuer(SpokeLifecycleLib.sol:295) until a newsetProbeExemptexecutes. Because noDepositsPausedSet(asset, false)orProbeExemptSet(asset, false)is emitted, monitors and frontends that rebuild state from events keep showing the asset as deposit-paused and probe-exempt after the relist.Recommendation
Preserve
depositPausedacross a relist: inaddHubAssetandcreateSpoke, carry over the existing value instead of writing false, so onlyunpauseDepositscan lift a pause. Either carryprobeExemptover the same way, or emitProbeExemptSet(asset, false)when the relist resets it, and emitDepositsPausedSetwhenever a listing changes that flag.Resolution
DollarStore Team: Fixed in cdf9380, as recommended. addHubAsset and createSpoke keep the stored depositPaused and probeExempt values when an asset is relisted, so only the unpauser can lift a deposit pause and the exemption survives. Since neither flag changes on a relist, no new event is needed for monitors to stay correct.
-
I-03 Informational A single dust queue position makes a dry
viarevert a spoke-to-spoke swap instead of being skipped DoS ResolvedDescription
swapSpokeToSpokewalks thevialist and sizes each via's spoke-to-hub leg (got) without regard to what the paired hub-to-spoke leg can pay. It skips a via only whengot == 0. Whengot > 0and the second leg pays less, it reverts the whole call instead of moving on to the next via. The first leg matches the opposite queue before reserves, andenqueueRemainderwaives the minimum order size for the first position in an empty line. So, while the offered asset's spoke has no reserve to fill it, anyone can plant a 1-unit position in thevia -> offerAssetline, and that turns an otherwise dry via into one withgot = 1.// src/libraries/SwapRouteLib.sol:204-208 (swapSpokeToSpoke) uint256 got = fillDirected(sell[i], offerAsset, via[i], remaining, true); if (got == 0) continue; // only a fully dry first leg is skipped uint256 paid = fillDirected(buy[i], via[i], wantAsset, got, true); if (paid < got) revert IDollarStore.InsufficientLiquidity(amountUnits - remaining + paid, amountUnits); // src/libraries/SwapRouteLib.sol:517 (enqueueRemainder) if (remaining < minOrder && q.positionCount != 0) revert IDollarStore.OrderTooSmall(remaining, minOrder);Take a swap of spoke asset S for spoke asset W routed through the launch hub assets
[USDC, USDS], at a moment when S's spoke holds no S reserve and no DLRS-side reserve above its minimum, W's spoke holds no W reserve, and the USDS route can complete the full amount through waiting USDS to S and W to USDS positions, so the USDC route fills nothing in either leg. Planting a 1-unit USDC to S position, which queues because no S reserve can fill it, makes the USDC leg take that unit. USDC cannot pay it out, so the call revertsInsufficientLiquidity(0, amountUnits), and the USDS route that would have filled is never tried. Because the call reverts, the dust is never consumed. It stays in the line until it is filled or cancelled, so one planted unit breaks every call through thatviaorder, not just one transaction. The attacker can cancel it at any time for a full refund, so the attack costs only gas.The documented advice to simulate multi-via swaps (docs/PROTOCOL.md) does not protect against this. A simulation that passes is still broken by dust planted before the transaction executes, and routers or integrations with a fixed
viaorder that do not re-simulate every call keep failing for as long as the dust stays. This conflicts with the project's own rule that "A single dust entry must not be able to strand a route" (docs/QUALITY.md §8). For affected callers, the call reverts and they pay the gas.Recommendation
Document in PROTOCOL.md and the
swapSpokeToSpokeNatSpec that a via whose first leg fills but whose second leg falls short reverts the whole call instead of being skipped, so callers should put the most liquid via first and simulate immediately before sending.Resolution
DollarStore Team: Fixed in 2d2e3d6, with a code change rather than the documentation recommended, because a planted dust position should not be able to strand a route. In swapSpokeToSpoke, each hub asset in via now carries at most what both of its legs can fill for the caller, using the same calculation as getSwapQuote. A hub asset that can carry nothing is skipped without settling anyone on its lines, and one that fails the inflow check is skipped instead of reverting the call. In the scenario from the finding, the dry hub asset is now skipped and the swap fills through the next one (test_s2s_dustBuyerOnShortHubAsset_skippedForNext).
Two limits remain and are documented in the NatSpec and PROTOCOL.md, with worked examples. Hub assets are used greedily in the caller's order, and they share the offered spoke's hub claim and the wanted spoke's reserve, so a list can revert where another order would fill; routers choose the order off-chain and simulate. Sizing assumes every payout succeeds, so a position waiting ahead whose owner is frozen counts against a hub asset until it is parked; processQueue parks it first.
Tests include a fuzz test against a brute-force best split of the amount across hub assets.
Remediation Review
4 findings · October 21 to 26, 2026-
I-01 Informational Production runbook lists hub assets before activating the launch cap initialization-ordering Reported
Description
The production runbook executes initial hub listings before scheduling the hub launch cap. The deployed hub starts unpaused with cap zero, meaning unlimited, so following that sequence admits permissionless deposits throughout the subsequent cap timelock delay. A checklist asking operators to verify caps before their first deposit cannot prevent third-party deposits. Both submitted tests pass using the real deployment routine and timelock. This is a runbook/bootstrap sequencing defect, not a bypass of an already-active cap: the existing hub can alternatively be capped before listing.
Impact: A third party can establish exposure above the intended launch budget before the cap activates; the PoC deposits four times that budget. The later cap leaves existing exposure unchanged and blocks further deposits while exposure remains above it. No theft or underbacking is demonstrated.
Recommendation
Configure the existing hub's cap before executing the first listing, or atomically execute cap configuration and listing in one timelock batch. Alternatively keep the protocol paused throughout bootstrap and unpause only after configuration. Update the production runbook accordingly.
-
I-02 Informational Donations can invalidate atomic timelock recovery batches and force a fresh delay denial-of-service Reported
Description
syncReserves reverts when actual custody no longer has negative drift. An attacker can repair an affordable deficit by transferring tokens directly to DollarStore after governance schedules an atomic recovery batch containing that asset and other still-required actions. The repaired asset's sync now reverts, rolling back the entire batch, including reconciliation of unrelated larger losses. Exact deficit repair creates no rescuable excess. The batch remains Ready, not consumed, but cannot execute while that sync has no negative drift. Removing the repaired call requires a newly scheduled payload and its configured delay; changing only the salt does not fix the stale payload. Both revised PoC tests pass, including rollback, operation-state assertions, replacement execution failing immediately before maturity and succeeding exactly at maturity, and a no-donation control.
Impact: A 1-unit donation blocks a particular batch reconciling a separate 400-unit loss and delays that work by another governor delay: seven days in the demonstrated reference configuration. Other bundled actions are also delayed. This requires a batch containing an affordable-to-repair deficit and still-needed work, not malicious governance. It does not establish a protocol-wide deadlock, cheap indefinite repetition, or additional principal loss. Invalidating a reduced replacement requires repairing its remaining deficit.
Recommendation
Make the no-negative-drift case an idempotent no-op while retaining escrow-impairment checks. Alternatively, schedule independent reconciliation operations so a repaired asset cannot block unrelated recovery work.
-
I-03 Informational swapSpokeToSpoke reports a hub pause or an unhealthy via as InsufficientLiquidity Unexpected Behavior Reported
Description
Since 2d2e3d6,
swapSpokeToSpokeskips a hub asset inviathat failsPegLib.canInflow: the asset is deposit-paused, the hub pool is paused, or its feed is missing, stale or off-peg.// src/libraries/SwapRouteLib.sol:198 if (!PegLib.canInflow(via[i])) continue;When the skipped entries leave the amount unfilled, the call ends in the generic shortfall revert (:219-224). A single-via call, or one where every via is unhealthy, therefore reverts
InsufficientLiquidity(wantAsset, 0, fullOutput)instead of naming the pause or price failure. Before 2d2e3d6, the route loop calledPegLib.checkInflow(via[i])(SwapRouteLib.sol:194 at f78ae37), which reverted with the specific error (DepositsPaused,PoolPaused(0),PriceStale, …).The docs describe these cases as pauses and price failures with their own errors:
- SECURITY.md:210 and :212 list
swapSpokeToSpokeamong the calls an asset pause or a hub pool pause blocks. - docs/PROTOCOL.md:274-275 map price failures and pauses to their own errors.
- docs/PROTOCOL.md:267 tells integrators to answer
InsufficientLiquidityby swapping a smaller amount or trying anotherviaorder.
No funds are at risk, and the swap is correctly refused. During a pause or oracle incident, though, routers retry smaller amounts, which fail the same way, or reorder
via. Monitoring also records the pause as a liquidity shortage.Found during the fuzzing campaign, while classifying expected reverts for the spoke-to-spoke handler.
Recommendation
Keep skipping unhealthy entries. When the call ends short and an entry was skipped for failing the inflow check, revert with that cause: for example, call
PegLib.checkInflowon the first skipped via before the shortfall revert, or revert a dedicated error such asNoHealthyVia(via).Alternatively, document at docs/PROTOCOL.md:267 that
InsufficientLiquiditywithfillableAmountOut == 0can mean every via was skipped because of a pause or a price failure, and state the same at SECURITY.md:210 and :212. - SECURITY.md:210 and :212 list
-
I-04 Informational The integrator error table names the wrong error for a hub exit above the caller's balance and omits the deposit errors Documentation Reported
Description
ae25d5a adds an error table for integrators. Its row for
InsufficientReceiptSharesreads "More DLRS or shares than the caller holds". Only a spoke exit reverts that way (SpokeLifecycleLib.sol:190). A hub exit burns the DLRS first, so areceiptAmountabove the caller's balance reverts with the token'sERC20InsufficientBalance(caller, balance, needed):// src/libraries/SpokeLifecycleLib.sol:184-190 if (poolId == 0) { DLRS(CoreStorage.layout().dlrs).burn(msg.sender, receiptAmount); // ERC20InsufficientBalance } else { ... if (owned < receiptAmount) revert IDollarStore.InsufficientReceiptShares(receiptAmount, owned); }previewWithdraw(0, ...)does revertInsufficientReceiptShares, but against the total supply, not the caller's balance (:64). An integrator that decodesInsufficientReceiptSharesto show "withdraw less" never sees it on a hub exit, and the table's last row, "An error from the token itself", describes a failed transfer, not a burn.The table also omits the errors a deposit can revert with. docs/PROTOCOL.md:51 lists the conditions in prose, but the errors are not named anywhere in the document:
AssetNotListed(DollarStore.sol:519),WrongPool(:523, :539),FeeOnTransferNotSupported(:813) andLaunchCapExceeded(:995). A spoke at its launch cap is the one an integrator meets in normal operation.Integrators building error handling from the table decode the wrong error on hub exits and have no entry for a deposit rejected by the launch cap.
Recommendation
Either check the caller's DLRS balance in the hub exit and revert
InsufficientReceiptShares(receiptAmount, balance)before the burn, so the table holds as written, or change the row to say a hub exit reverts with the DLRS token'sERC20InsufficientBalance. Add rows forAssetNotListed,WrongPool,FeeOnTransferNotSupportedandLaunchCapExceeded(poolId, attempted, cap).
No findings match.
Put your code through the same review.
This review started with a conversation about scope. Tell us what you are building and we will plan yours with you.
