Skip to content

Lossless decimals conversion assumes vault rate is exactly representable; reverts (DoS) for the common non-1:1 vault #70

Description

@thedavidmeister

File: src/lib/erc4626/LibERC4626.sol:36-43, 55-62
Severity: HIGH · Dimension: pass6

HAZARD: convertToAssets/convertToShares route every value through LibDecimalFloat.toFixedDecimalLossless(amount, decimals) on the way IN and fromFixedDecimalLosslessPacked(raw, decimals) on the way OUT. The word silently 'works' today only because the chosen test/fork vaults are (near-)1:1 with 18-decimal shares, so the round-trip happens to be lossless. SHAPE THAT CREATES IT: the design hard-codes the lossless variant as the default conversion path, baking in the assumption that shareDecimals==assetDecimals (or that the rate divides evenly). The lossless variant reverts whenever a value cannot be represented exactly in the target decimal precision. PRODUCTION SCENARIO: a real ERC-4626 vault with 18-decimal shares over a 6-decimal asset (USDC, the most common stablecoin vault) at any non-trivial exchange rate produces a shares/assets value with sub-asset-decimal precision; toFixedDecimalLossless then reverts. The fork suite only tests vaults that return exactly 1e18 (or the one SGOV case that still happens to land lossless), so the gap never fires in CI and every Rainlang order that uses erc4626-convert-to-* against such a vault reverts on-chain — the user's strategy silently stops executing. STRUCTURAL FIX: this is a redesign, not a comment — decide the rounding contract explicitly (use the rounding/saturating decimal-float conversion with a documented round direction, mirroring how ERC-4626 itself specifies floor/ceil for convertTo*), and add a property/fuzz test over (shareDecimals, assetDecimals, rate) asserting the word returns a value rather than reverting for representative decimal mismatches.

Proposed fix

Replace the toFixedDecimalLossless / fromFixedDecimalLosslessPacked pair on the conversion path with the rounding (non-lossless) LibDecimalFloat conversion with an explicit, documented round direction, and add a fuzz test sweeping share/asset decimals (e.g. 6/8/18) and non-unity rates asserting non-reverting, correctly-rounded output.


Filed from the rain.erc4626.words whole-repo audit (/audit:audit). The shipped code was confirmed correct by adversarial mutation testing; this is a coverage / quality / hazard finding, not a runtime bug.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

auditaudit findinghighpass6audit dimension 6

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions