fix(erc4626): accept Sky/Spark referral deposit/mint overloads (ENG-3841)#31
fix(erc4626): accept Sky/Spark referral deposit/mint overloads (ENG-3841)#31ajag408 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Risk: medium. Cursor Bugbot and Cursor Security Agent completed successfully with no reported findings. Risk exceeds the low approval threshold due to shared base-validator calldata integrity changes and expanded SUPPLY acceptance, so I am not approving; human reviewers are already assigned.
Sent by Cursor Approval Agent: Pull Request Router and Approver
| 'function deposit(uint256 assets, address receiver) returns (uint256)', | ||
| 'function mint(uint256 shares, address receiver) returns (uint256)', | ||
| // Sky/Spark Savings referral overloads (sUSDS, sUSDC, sDAI …). | ||
| 'function deposit(uint256 assets, address receiver, uint16 referral) returns (uint256)', |
There was a problem hiding this comment.
one scope question on this overload: since it is added to the shared ERC4626_ABI, deposit/mint(..., uint16 referral) becomes accepted for every supported generic vault, not only sky/spark, the new test also validates it against the euler fixture...should this be gated by protocol/selector so generic vaults that do not support this overload keep rejecting it?
There was a problem hiding this comment.
Good question. considered gating it, and went accept-everywhere deliberately. Reasoning:
-
Security-wise the overload is inert. Everything that protects funds on the 2-arg path runs identically on the 3-arg path: tx.to must be a whitelisted vault, from == user, receiver == user (or the declared receiverAddress), amount > 0, no ETH value, and the byte-exact tamper re-encode. The extra arg is a uint16 — it can't encode an address or redirect anything. On Sky/Spark it only feeds a Referral event for ops attribution
-
For vaults that don't implement the overload, the tx just reverts on-chain — selector 0x9b8d6d38 doesn't exist on a vanilla ERC-4626, so no state changes and no funds move. A reverting deposit to a whitelisted vault is the same exposure class as any failed deposit. (If a whitelisted vault had a malicious fallback that swallowed unknown selectors, we'd have a bigger problem than this overload. The whitelist already assumes vault contracts are what they claim.)
Strict protocol specific comparison is possible but doesn't provide added security benefit and any protocols extending support to this referral code param will need a Shield update/result in false blocks.


Summary
Fixes a production false block reported by DFNS: entering the sUSDS vault
(
ethereum-usds-susds-0xa3931d71877c0e7a3148cb7eb4463524fec27fbd-4626-vault)failed on the SUPPLY step with
No matching operation pattern found.The monorepo recently switched sUSDS deposit construction to Sky's referral
overload —
deposit(uint256 assets, address receiver, uint16 referral),selector
0x9b8d6d38— which the ERC-4626 validator's ABI didn't register, sothe calldata failed to parse. Same overload exists on Spark Savings vaults
(referral 200 on L2s vs 3008 on Ethereum).
Changes:
erc4626.validator.ts— adddeposit/mint(…, uint16 referral)overloads to
ERC4626_ABI. All existing SUPPLY checks (receiver == user,amount > 0, no ETH value, vault whitelist, calldata integrity) run unchanged
on the 3-arg path since
assets/receiverkeep their positions.base.validator.ts— tamper check now re-encodes withparsedTx.fragmentinstead ofparsedTx.name. ethers v6 throws onname-based lookups for overloaded functions, so without this the overloaded
ABI would block all deposits/mints (2-arg included) with
"Failed to validate calldata integrity". Behavior-neutral for every other
validator — the fragment is always the one the calldata parsed as.
erc4626.validator.test.ts: 3-arg deposit passes for referral3008 / 200 / 0 / 65535 (accept-any), 3-arg mint passes, receiver tampering /
appended-bytes tampering / zero amount / non-whitelisted vault all still
block.
handler.test.ts: end-to-end repro of the client's exact rejectedtransaction against the real vault registry → SAFE, plus a receiver-redirect
variant → blocked.
Decisions (per ops context):
attribution (they emit a
Referralevent, never route funds) and vary bychain/project (3008 Ethereum, 200 L2s, per-project dynamic codes planned).
injects the ref code internally, so Shield only ever sees 2-arg calldata on
that path.
branch rebases onto the 1.4.0 amount-validation PR currently in review.
Version:
1.4.0 → 1.4.1(patch — restores validation of a legitimate,already-whitelisted flow).
QA Proof
Local E2E via the built CLI (
dist/cli.js, samehandleJsonRequestentry npmand binary consumers hit), driven by
scripts/qa-eng-3841.mjs:Cases covered: client's exact rejected transaction → SAFE /
SUPPLY;receiver-tampered word → blocked; non-whitelisted
tx.to→ blocked; plain2-arg deposit (selector
0x6e553f65) → SAFE (regression); ticket's USDSAPPROVAL tx → SAFE /
APPROVAL(regression). Full unit + handler suites green.What Needs to Be QA'd in Staging
appended calldata bytes)
base.validator.tstamper-check change)QA Team Notification
Note
Medium Risk
Touches shared calldata integrity logic in
BaseEVMValidatorand expands which SUPPLY calldata is accepted, though referral values are unconstrained and existing receiver/amount/vault checks remain.Overview
Fixes false SUPPLY blocks for Sky/Spark savings vaults that call
deposit/mintwith auint16referral (e.g. sUSDS selector0x9b8d6d38), which the ERC-4626 validator could not parse before.The ERC-4626 ABI now includes the 3-argument
deposit/mintreferral overloads; existing SUPPLY rules (receiver, amount, vault whitelist, no ETH) still apply becauseassetsandreceiverstay in the same argument positions. Referral codes are not constrained.Calldata tamper re-encoding in
BaseEVMValidatornow usesparsedTx.fragmentinstead of the function name so ethers v6 can distinguish overloaded signatures without breaking integrity checks for all EVM validators.Adds unit coverage for referral deposits/mints and an end-to-end JSON handler repro of the client’s rejected sUSDS transaction. Version 1.4.0 → 1.4.1.
Reviewed by Cursor Bugbot for commit eca2a8f. Configure here.