fix(EXSC-423): address Vault Wrapper V1 peer-review feedback - #2296
Conversation
solhint's max-states-count excludes both constants and immutables (lib/rules/best-practices/max-states-count.js filters on `!isDeclaredConst && !isImmutable`), and the contract declares exactly 15 counted state variables against a limit that only trips above 15. The pragma has been inert since `factory` became the `FACTORY` immutable; its justification comment also miscounted, naming a 16th declaration that does not exist. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ory copy `_payIntegrators` copied the whole `integratorFeeReceivers` array into memory before the loop. The SLOAD count is identical either way — each receiver packs into one slot, and `wallet`/`bps` are read from the same slot — so the copy only added per-element allocation and MSTOREs. Measured on a cold-storage benchmark of the loop: -198 gas at 1 receiver, -2695 at the 50-receiver cap, per fee pool. `distributeFees` pays two pools. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sumption The trust model covered curation and the standard-ERC4626 requirement but not honest price reporting. A source that misreports price per share for a single block both charges a performance fee on a phantom gain and parks the up-only high-water mark above real price, so later genuine gains accrue no fee. Accrual runs on the permissionless distributeFees, so the moment is attacker-selectable. Records the accepted bound and names curation as the only control. Raised as INFO in peer review of #2092. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_proxyInitCode() embeds `beacon` in the init code that fixes every instance address, and `beacon` lives in factory proxy storage. It is write-once today (set in initialize, no setter), but a timelocked factory upgrade could add one, and changing it after the first deploy would silently relocate every predicted address and break cross-chain parity. Also records why the CREATE2 salt preimage is deliberately unversioned: it is abi.encoded, a repeated triple reverts rather than colliding, and `_nonce` already disambiguates. Raised in peer review of #2092. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| uint256 distributed; | ||
|
|
||
| for (uint256 i; i < count; ++i) { | ||
| FeeReceiver storage receiver = integratorFeeReceivers[i]; |
There was a problem hiding this comment.
Needs to be memory. Sorry for the confusion here. This reads the storage slot twice incurring additional gas costs compared to prior. 😅
There was a problem hiding this comment.
No.....
You have 2 options:
Either you do:
FeeReceiver[] storage receivers = integratorFeeReceivers;
...
FeeReceiver memory receiver = receivers[i]or straight
FeeReceiver memory receiver = integratorFeeReceivers[I]This is a nit anyway. 😅
There was a problem hiding this comment.
Done in d24c14bd8 , went with your second option
There was a problem hiding this comment.
I can't believe that the storage pointer version is cheaper at lower runs…
…ot a memory copy" This reverts commit bcd6ada. Measurement showed the whole-array copy, the storage pointer, and a per-element memory copy all land within 0.1% of each other on distributeFees (976 gas apart at the 50-receiver cap, 80 at a single receiver, on a call dominated by ERC-20 transfers). Not worth a review round-trip or the audit attention, so the loop goes back to the form on dev-vault-wrapper and this PR keeps only the unambiguous fixes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gvladika
changed the title
github-actions
Bot
added
the
requires-types
🤖 GitHub Action: Security Alerts Review 🔍🟢 Dismissed Security Alerts with Comments 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: 🟢 View Alert - File: ✅ No unresolved security alerts! 🎉 |
…t loop
Drops the whole-array storage-to-memory copy in `_payIntegrators` and reads
one entry at a time instead, as asked for in review. `FeeReceiver` packs
into a single 32-byte slot (wallet@slot0+off0, bps@slot0+off20), so an
entry costs one SLOAD and both fields are then read from memory — where the
whole-array copy paid per-element allocation on top of the same SLOAD, and
a storage pointer (the earlier attempt, since reverted) paid a warm re-read
of the slot.
Measured on distributeFees, both fee pools:
receivers whole-array storage ptr per-entry
1 108,115 107,912 107,992
5 220,568 220,228 220,220
50 1,485,700 1,483,783 1,482,807
Per-entry wins from five receivers up. The spread is under 0.1% either way
on a call dominated by ERC-20 transfers, so this is a readability-neutral
tidy-up rather than a meaningful saving.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
gvladika
changed the title
3 participants
Which Linear task belongs to this PR?
Ref EXSC-423 — S15, Security review & audit support.
Why did I implement it this way?
Addresses the peer-review comments from @reednaa on #2092. Targets
dev-vault-wrapperso the fixes land in the audited slice.His review verdict was that the code is defensible with no issues found — the report was 1 INFO plus a couple of GAS notes. This PR lands the two changes that are unambiguous; the four gas threads are answered on #2092 rather than changed, with reasoning below.
1.
chore: drop the dead solhintmax-states-countdisable@reednaa suspected the pragma was left over from when
factorywas storage rather than theFACTORYimmutable. Confirmed three ways:solhint/lib/rules/best-practices/max-states-count.jsfilters on!isDeclaredConst && !isImmutable, so immutables do not count.bunx solhint src/VaultWrapper/LiFiVaultWrapper.solreports nomax-states-countproblem with the pragma removed.The justification comment above it was also wrong on its own terms — it named a 16th declaration that does not exist, and
shareDecimalsOffsetis the 6th, not the 16th.2.
docs: underlying price-per-share is a trust assumptionRecords the INFO from
LibVaultWrapperMath.sol:163. The wrapper takes the source'stotalAssetsat face value, so a source misreporting price per share for even one block distorts the performance fee in both directions: the spike charges a fee on a gain that never existed, and because the high-water mark ratchets up-only to the spiked price, genuine later gains then accrue no fee until they exceed it. Accrual runs on deposit/withdraw and on the permissionlessdistributeFees, so the moment is attacker-selectable.Root cause is always in the underlying and the only control is curation, so this is a documented accepted bound rather than a code change. The trust model previously covered "sources are curated" and "must be standard ERC-4626" but never honest price reporting.
3.
docs: the beacon is address-parity-critical_proxyInitCode()embedsbeaconin the init code that fixes every instance address, andbeaconlives in factory proxy storage. It is write-once today (set ininitialize, no setter), but the factory now sits behind aTransparentUpgradeableProxy, so a timelocked upgrade could add one — and changing it after the first deploy would silently relocate every predicted address and break parity against already-deployed chains.Also records why the CREATE2 salt preimage is deliberately unversioned (the
LiFiVaultWrapperFactory.sol:375thread): it isabi.encoded so there is no packing ambiguity, a repeated (namespace, adapter, underlying) triple makesCreate2.deployrevert on the occupied address rather than colliding into one, and_noncealready serves as the disambiguator.4.
perf: copy each receiver entry into memory in the payout loop_payIntegratorscopied the wholeintegratorFeeReceiversarray into memory up front; it now reads one entry at a time.FeeReceiverpacks into a single 32-byte slot (forge inspect:wallet@slot0+off0,bps@slot0+off20), so an entry costs one SLOAD and both fields then come from memory.Commits 2 and 4 in this branch are an earlier attempt at this and its revert — that attempt used a
storagepointer, which pays a warm re-read of the same slot, and @reednaa correctly rejected it. Left in history rather than squashed so the review thread still lines up.Measured on
distributeFees(both fee pools, real contract, cold and warm — the deltas are identical in both):Per-entry wins from five receivers up. The whole spread is under 0.1% on a call dominated by ERC-20 transfers, so this is a tidy-up, not a meaningful saving.
Two things left alone deliberately:
uncheckedon that loop is sound — see below — but not worth swapping a compiler-enforced invariant for a prose one here.On
unchecked: the boundedness argument holds —_setIntegratorFeeReceiversis the sole writer and admits 1..50 entries whose bps sum to exactly 10000, soiis bounded, each non-last share is≤ _integratorTotal,distributedis a sum of floors that cannot exceed it, andretained ≤ distributed. Not applied: ~260–400 gas at realistic receiver counts, in exchange for replacing a compiler-enforced invariant with a prose one in the fee-distribution loop, with the audit as the next gate.Checklist before requesting a review
No new tests: the Solidity changes are deleting an inert lint pragma and a storage-access refactor with identical semantics, whose fan-out, last-receiver remainder and retain-failed-payout paths are already covered in
VaultWrapperDistribution.t.sol. The full VaultWrapper suite (377 tests) passes unchanged. The gas probes used for the measurements above were throwaway and are not committed.Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)