fix: Bailsec Core audit remediation (breaking: redemption ABI) - #33
Merged
Merged
Conversation
added 22 commits
September 7, 2026 09:34
Covers commit 0ce59af across the parallelizer and tokens repositories.
harvest forwards caller-supplied router calldata and refreshes the collateral oracle, so it cannot be exposed to arbitrary callers. This aligns GenericHarvester with MultiBlockHarvester, which already gates both of its entry points. Closes Bailsec Core Issue_04, Issue_05 and Issue_07.
updateOracle raised the stored target to the current spot with no bound on the increase, and emitted nothing, so a transient upward print could be locked in permanently and left no trace in the logs. A single update is now capped at MAX_ORACLE_RATCHET_STEP and emits OracleTargetUpdated. Failing to ratchet is the conservative direction: readMint clamps at the lower of spot and target, and readBurn cannot raise a stale target above its stored value. Part of Bailsec Core Issue_04.
setMaxSlippage wrote to maxTokenSlippage, a mapping no harvest path reads: every live check uses yieldBearingData[asset].maxSlippage. A governance tightening therefore appeared to succeed on chain while leaving the effective limit untouched. The setter now writes the parameter the harvests read, the dead mapping is removed, and the bound matches setYieldBearingAssetData: 1e9 means zero slippage protection and is rejected. Closes Bailsec Core Issue_03.
…ement Two defects in onFlashLoan, both already carried by the unmerged rebalancer work and ported here so the audit remediation is complete on its own. The router was assumed to consume the full declared input while only the output balance was checked. A partial fill left both the unspent input and its standing allowance on the contract, outside the Parallelizer's accounting. The input balance is now compared across the swap. Settlement was also asymmetric: a shortfall was charged to the initiating caller while a surplus stayed as an unattributed contract balance the caller could not withdraw. The positive branch now credits the caller. Closes Bailsec Core Issue_07 and Issue_08.
The contracts adjust the Parallelizer's exposure between a yield bearing asset and its underlying, they do not collect yield, so Rebalancer is the accurate name. It also aligns this repository with the contracts already deployed under that name on Base and Avalanche. Renamed: BaseHarvester, GenericHarvester and MultiBlockHarvester, along with their deploy scripts, configuration keys and registry entries. The IHarvester interface and the legacy ContractType.Harvester entry keep their names so registry ordinals are unchanged, matching the deployed contracts. Existing deployment records are left untouched: they document what was deployed under the old name.
…ords Completes the rename: the interface follows the contracts that implement it. The upstream reference to Angle's IHarvester is left as written since it names a file in their repository. The GenericHarvester deployment records are removed. They are internal helper contracts, no code resolves them by name, and the entries would otherwise point at an implementation this repository no longer builds.
The rename also rewrote the references to Angle's own files, so the attribution named contracts that do not exist upstream and the links no longer resolved. Angle's files are BaseHarvester, GenericHarvester and MultiBlockHarvester, and the notices now name them again. BaseRebalancer and GenericRebalancer are marked as substantially modified, which is accurate for both; MultiBlockRebalancer is unchanged beyond its name and keeps the plain notice.
The previous check rejected any route that consumed less than it was offered. That is too strict: the input is sized on chain from the live Parallelizer state while the router calldata is built off chain, so the two cannot be expected to match exactly, and any upward drift between the keeper's simulation and inclusion would have failed the rebalance. The remainder is now swapped back through the Parallelizer and added to the settlement, so it reaches the initiating caller's budget rather than being stranded. The router allowance is also cleared after the swap so a partial fill leaves nothing standing. Refines Bailsec Core Issue_07.
onFlashLoan had no coverage at all, so neither the router leg nor the budget settlement was exercised. A zero-fee ERC-3156 lender mock lets the existing MockRouter drive a route that consumes less input than it was offered. Without the recovery the run leaves 781.198 of the input stranded on the rebalancer; the test asserts nothing is left, no router allowance survives, and the recovered value reaches the caller's budget.
…hind Rebalancing grants unlimited allowances and never reduces them, while rotating a yield bearing asset or a deposit address only overwrote the configuration. The outgoing spender kept authority over any of that token the contract later held, for the rest of the contract's life. Both rotations now revoke the approval they made obsolete, and governance gets resetAllowance as the escape hatch for anything else, as recommended. Closes Bailsec Core Issue_02.
`onFlashLoan` refused any callback reporting a non-zero fee, so enabling the USDp flash loan fee would have left the rebalancer with no working path to adjust the Parallelizer's yield bearing exposure. The fee is now folded into what the lender pulls back and settled against the budget of the address that called `harvest`, alongside the usual shortfall or surplus. A fee the caller cannot cover fails the rebalance rather than reaching into another caller's budget. Bailsec Core Issue_06
FlashParallelToken stores `feesRate` as a uint16 in basis points, capped at 100%. The mock scaled by 1e9 instead, so its rates read five orders of magnitude away from anything governance could actually set. The budget shortfall case now uses that real ceiling against a route that clears `minAmountOut` with nothing spare, rather than a fee rate the live contract would reject.
`minAmountOuts` is positional and the output list is rebuilt at execution time from the live `collateralList`. A revoke (swap and pop) followed by an add preserves the length while reshuffling the tokens, so each minimum ends up guarding a token the caller never quoted. The three redeem entry points now take an `expectedTokensHash` over the output list, checked against the live one, and the authorized path folds it into the derived EIP-3009 nonce so a relayer cannot hold a signature until governance rotates collateral. This changes the ABI of `redeem`, `redeemWithForfeit` and `redeemWithAuthorization`, and invalidates any redemption signature outstanding at upgrade time. Bailsec Core Issue_09
…endence `invariant_PathIndependenceCollateralRatio` failed once during the audit work at 0.2176% against a 0.2% tolerance. The driver was not the documented one. `ArbitragerWithSplit` split a redemption that forfeited tokens. Forfeited tokens stay in the reserve while the others shrink, so the composition drifts and each later redemption forfeits a larger share of value. Measured against a plain redemption split the same way: splits 2 4 8 16 no forfeit 0 0 0 0 bps forfeit B 193 289 337 360 bps forfeit B+Y 146 209 238 252 bps So a single split already sat an order of magnitude above the tolerance, and the suite only passed because the fuzzer rarely landed a large forfeited redemption. The comment credited non-flat fee curves and the entry-time penalty, both of which measure zero here. Forfeit is what breaks the comparison when split, not forfeit itself, so it moves to its own `redeemWithForfeit` handler that applies the same unsplit redemption to both systems. They drift together, the forfeit branch and its payout assertions stay covered, and only the plain redemption is split. Across 25 seeded campaigns the residual divergence peaks at 1.4e-8%, down from 4.1e-6%, so the collateral ratio tolerance drops from 0.2% to 0.01%, matching the total supply one.
`maxSlippage` was a uint96, a width inherited from the Angle fork where it was a standalone variable that had a slot to itself. Inside `YieldBearingParams` its 12 bytes no longer fit beside the four uint64 exposures, so it spilled into a third slot and left 20 bytes unused. The value is bounded below 1e9, so uint64 is ample and closes the second slot exactly: asset slot 0 offset 0 targetExposure slot 0 offset 20 maxExposure slot 1 offset 0 minExposure slot 1 offset 8 overrideExposures slot 1 offset 16 maxSlippage slot 1 offset 24 Configuring an asset drops from 104588 to 82501 gas.
`sellRewards` forwarded its payload to `ODOS_ROUTER`, a constant holding the Ethereum Odos address. The same facet bytecode ships to every chain, so the target was wrong everywhere but mainnet. Checked on the five live deployments: mainnet bytecode present, the Odos router sonic bytecode present, an unrelated contract base no bytecode avalanche no bytecode hyperevm no bytecode A raw call to an address with no code returns success with empty returndata, so the flow ran on to find no balance increase and reverted with InvalidSwap. The selector is wired on all five diamonds. The target moves to `swapRouter`, appended to `ParallelizerStorage` and set by governance. Nothing is Odos specific: the payload is executed with a raw call, so any aggregator or DEX can be configured. `ODOS_ROUTER` is gone, `OdosSwapFailed` becomes `RewardSwapFailed`, and `MockOdosRouter` becomes `MockSwapRouter`. An unset router now reverts instead of calling address zero, which would have reported success and left the swap silently unperformed. Layout.sol had drifted: it documented 9 slots while the struct had grown to 15 with the surplus distribution fields. It now mirrors all 16, and the layout test asserts the tail so an append cannot slip through unnoticed again. Governance must call `setSwapRouter` on every chain after the upgrade, before rewards can be sold. Bailsec Core Issue_15
Bailsec Core Issue_10 records the remediation as smoothing the redemption curve to prevent profitability. The curve live on all five chains is indeed shallow: collateral ratio 75% 85% 95% 97% payout factor 99.5% 95% 95% 99.5% A 4.5 point band, against the 40 point swing the finding illustrates. It raises the bar without closing it. Against those exact parameters, with a second holder present so the transfer is visible and a correctly priced donation asset: attacker share 25% 30% 35% 40% 50% net result -444 -133 +178 +489 +1111 Break-even sits between 30% and 35% of supply. At 50% the attacker gains 1111 while the other holder loses 1106, so it is a transfer, not an artefact. No overvalued collateral is needed, unlike the finding's illustration. What decides profitability is not that a deterrent exists but how steeply it climbs back. The attacker pays in ratio points and is repaid in payout points on their own redemption, recovering the share `f` they still hold a claim on, so a ramp starves the attack when its slope stays below (1 - f) / (f * H/S). Holding the same 4.5 point deterrent and widening the band: slope 0.900 0.450 0.225 0.112 attacker at 50% loses loses loses loses attacker at 90% GAINS GAINS GAINS loses Measured thresholds match the formula: about 1.05 against half the supply, about 0.12 against ninety percent. The deployed curve sits at 2.25. So the deterrent does not have to be given up. Ramping the same 4.5 points from a 60% ratio up to 100% lands at slope 0.112 and starves the attack even against a holder of ninety percent of the supply, while removing the current shape's oddity of paying its best rate at the deepest undercollateralization. Flattening also works, and `y[last]` prices every redemption above a 100% ratio, so it has to flatten at 99.5%: flattening at 95% would cost healthy redeemers 4.52% instead of 0.5%. Bounds are asserted rather than only printed, so changing the curve fails these tests and forces the trade-off to be re-measured.
The deployed curve returned to 99.5% over two ratio points, a slope of 2.25. An attacker pays in ratio points and is repaid in payout points on their own redemption while recovering the share they still hold a claim on, so a donation pays whenever that slope exceeds (1 - f) / (f * H/S): about 1.05 against half the supply, about 0.12 against ninety percent. x 75% 85% 95% 97% -> 0 60% 100% y 99.5% 95% 95% 99.5% -> 95% 95% 99.5% The same 4.5 point deterrent now spans a 60% to 100% band, a slope of 0.112, under the threshold even for a holder of ninety percent of the supply. `y[last]` stays at 99.5% because it also prices every redemption above a 100% ratio. Ramping monotonically also drops the previous shape's oddity of paying its best rate at the deepest undercollateralization. Carried in the six deploy configs for fresh deployments, and in `SetRedemptionCurve.s.sol` for the five live ones, one run per chain. Bailsec Core Issue_10
Bailsec Core Issue_22 asks that fees be set so `userDeviation` cannot be
exploited. Reading the code, the finding points instead at a live configuration
the contract already refuses to accept.
`userDeviation` replaces a spot inside its band with the target for both mint and
burn. So `readMint` credits a discounted collateral at face value, and because
the snap runs before the burn firewall, the collateral stops contributing any
depeg haircut while it sits inside the band.
`setOracle` already guards this:
if (userDeviation > burnRatioDeviation) revert InvalidParams();
The invariant is exactly right: while the snap applies the firewall would not
have fired anyway, so the snap can never suppress it. Two mainnet collaterals
violate it, pairing a 5 bps snap band with a zero firewall band, and they predate
the check. `DiamondInitializer` routes through the same `setOracle`, so the
shipped config would no longer deploy.
The tolerance moves to the burn side, matching the two collaterals already
configured that way. Measured, nothing is lost:
mint pricing firewall 4 bps firewall 20 bps
before face value, discount taken suppressed suppressed
after true spot, discount to minter tolerated engages
Seven collateral entries across five chains corrected in the deploy configs.
Governance must apply the same change on chain through `setOracle`.
`_addPayee` validated only that the share was non-zero. The Parallelizer holds the income being distributed, so a share allocated to itself is handed straight back to `release` as fresh income on the next call, redistributing itself. `address(0)` deliberately stays valid. `LibSurplus._release` treats it as the sentinel for burning that share, and the live surplus policy uses it. An earlier draft rejected both and broke the surplus tests, which is how the sentinel surfaced. Bailsec Core Issue_19
`bun run lint:sol` reports it as an error rather than a warning, so it would fail CI. Introduced by the Issue_04 fix.
FabienCoutant
force-pushed
the
fix/bailsec-core-audit
branch
from
September 8, 2026 14:22
d2adb18 to
95c5ccb
Compare
added 4 commits
September 28, 2026 10:08
…at zero The ratchet bounds each step against the stored target, so a target left at 0 can never be raised: every positive spot exceeds 0 by more than 5%. New chains would deploy with a permanently frozen oracle. The initializer now seeds the target from spot once, and the ratchet governs every later move. Seeding is refused on an already-set target, so it cannot be used to sidestep the bound. Reported by Bailsec against the Issue_04 remediation.
…tates Asset rotation cleared `oldUnderlying -> yieldBearingAsset`, the vault path's relationship. The MultiBlock deposit path instead approves `underlying -> depositAddress`, so that allowance survived the rotation and the regression test asserted the same wrong pair. A virtual hook lets each rebalancer revoke what it actually granted. The new test fails without the override. Reported by Bailsec against the Issue_02 remediation.
…steady state processSurplus drives the ratio to surplusBufferRatio, 100.5% on mainnet, so the margin over the profitable region is under a point rather than the headroom a spot reading suggests. The measurements also correct the earlier framing: the break-even sits just under 100% whatever the ramp, so the defence is the cap at 100% rather than the slope. Widening the ramp or trimming the deterrent barely moves it.
…lus cycle processSurplus runs monthly, so the ratio sawtooths between the buffer and whatever yield adds before the next run. The trough is the block the run lands in, which is both the thinnest and the most predictable moment. Both ends of the cycle stay loss-making, and by almost the same margin: above 100% the curve is capped, so extra headroom buys nothing. That is the cap doing the work rather than the size of the buffer.
FabienCoutant
force-pushed
the
fix/bailsec-core-audit
branch
from
September 28, 2026 10:07
d802c9b to
3e7d93a
Compare
added 2 commits
September 28, 2026 13:47
solhint caps lines at 120 and the two GenericRebalancer constructions ran to 122, which failed the lint job and skipped the test jobs behind it.
Shortening the facet names left the markdown columns unaligned, which failed the prettier check in the lint job.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remediation for the Bailsec Core first report (September 2026). All 46 findings carry a response,
sent to Bailsec separately for their comments and resolution columns.
Breaking change
redeem,redeemWithForfeitandredeemWithAuthorizationtake a newexpectedTokensHash, so theirselectors change and every redemption signature outstanding at upgrade time is invalidated.
Integrators must move to quote, hash, redeem. This is Issue_09, and the report anticipated the break.
Fixed in code
resetAllowanceaddedsetMaxSlippagewrites the field the rebalance reads; the deadmaxTokenSlippagemapping is goneharvestrestricted to trusted callers; MAX oracle ratchet bounded at 5% per update and loggedRewardHandlercalls a governance-setswapRouterinstead of a hardcoded Ethereum addressEach fix has a regression test that fails without it.
Fixed by configuration
Issue_10. The deployed redemption curve still made the donation attack pay above roughly a third
of supply, with no overvalued collateral needed. Profitability is set by the slope of the segment
climbing back to
y[last], so the same 4.5 point deterrent now spans a 60% to 100% ratio band.Slope goes from 2.25 to 0.112.
Issue_22.
setOraclealready enforcesuserDeviation <= burnRatioDeviation. Two mainnetcollaterals violate it because they predate the check, which means the shipped config would not
deploy. The tolerance moves to the burn side.
Also in here
The
Harvestercontracts are renamed toRebalancerto match what is deployed,YieldBearingParamspacks into two slots instead of three, and the path independence invariant now measures path
independence: it was comparing split forfeited redemptions, which diverge by design.
Governance actions after merge
Three calls on each of the five chains, none performed by the upgrade:
setRedemptionCurveParams, the Issue_10 rampsetOracle, the Issue_22 deviation pairingsetSwapRouter, without whichsellRewardsrevertsStill outstanding
The live Rebalancers on Base (
0xa130adc5) and Avalanche (0x45c8db32) run unaudited code withoutIssues 03, 04 and 05. They are only covered once redeployed from this branch.
Verification
53 test suites, 415 tests, no failures.
forge fmt --checkreports fewer diffs thandev, andsolhintreports no errors.