Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an optional Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/validators/evm/erc4626/erc4626.validator.test.ts (1)
664-760: Add one JSON-entrypoint test forreceiverAddress.These cases validate
ERC4626Validatordirectly, but the public API path still depends onsrc/json/schema.tsandsrc/json/handler.tsaccepting and forwardingargs.receiverAddress. One handler-level test here would catch schema/forwarding regressions that these unit tests cannot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/validators/evm/erc4626/erc4626.validator.test.ts` around lines 664 - 760, Add a JSON-entrypoint integration test that constructs a JSON request which includes args.receiverAddress and sends it through the public JSON path (the schema/handler code) so schema.ts/handler.ts will parse and forward args to the ERC4626Validator; specifically, add a test that builds the same calldata/data and tx object used in these unit tests, invokes the JSON handler (via the public export in src/json/handler.ts, e.g., handler or handleRequest) after validating/parsing with src/json/schema.ts, and assert the response mirrors validator.validate(tx, TransactionType.SUPPLY|WITHDRAW, USER_ADDRESS, { receiverAddress }) (isValid true/false and reason when blocked) to catch any schema/forwarding regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/validators/evm/erc4626/erc4626.validator.ts`:
- Around line 128-139: Summary: Blank-string receiverAddress values are
currently treated as valid overrides because of the "??" fallback, causing
receiver mismatches; explicitly reject empty-string overrides. Update the
routing/validation: in the switch block keep passing receiverAddress, but inside
validateSupply and validateWithdraw (and any other places that accept
receiverAddress) add an explicit check that if receiverAddress === '' then fail
validation (return an error/throw/ValidationResult) with a clear message about
blank receiver overrides being forbidden; otherwise treat undefined/null as "use
userAddress" (i.e., only null/undefined fall back, not empty string). Ensure the
error path uses the same validation/error type returned by
validateSupply/validateWithdraw so callers (the switch) handle it consistently.
---
Nitpick comments:
In `@src/validators/evm/erc4626/erc4626.validator.test.ts`:
- Around line 664-760: Add a JSON-entrypoint integration test that constructs a
JSON request which includes args.receiverAddress and sends it through the public
JSON path (the schema/handler code) so schema.ts/handler.ts will parse and
forward args to the ERC4626Validator; specifically, add a test that builds the
same calldata/data and tx object used in these unit tests, invokes the JSON
handler (via the public export in src/json/handler.ts, e.g., handler or
handleRequest) after validating/parsing with src/json/schema.ts, and assert the
response mirrors validator.validate(tx, TransactionType.SUPPLY|WITHDRAW,
USER_ADDRESS, { receiverAddress }) (isValid true/false and reason when blocked)
to catch any schema/forwarding regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f3b1039a-6855-4fa2-8507-42db4aa4b437
📒 Files selected for processing (5)
package.jsonsrc/json/schema.tssrc/types/index.tssrc/validators/evm/erc4626/erc4626.validator.test.tssrc/validators/evm/erc4626/erc4626.validator.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/validators/evm/erc4626/erc4626.validator.ts (1)
128-128:⚠️ Potential issue | 🟡 MinorHandle
receiverAddressexplicitly on Line [128] instead of boolean coercion.
args?.receiverAddress || undefinedsilently treats''as absent and keeps a strict-null warning alive. Explicitly reject blank overrides (or normalize with explicit branches) so malformed intent isn’t silently defaulted.🔧 Proposed fix
- const receiverAddress = args?.receiverAddress || undefined; + const receiverAddress = args?.receiverAddress; + if (receiverAddress !== undefined && receiverAddress.trim() === '') { + return this.blocked('receiverAddress must be a non-empty string'); + }#!/bin/bash set -euo pipefail echo "== Verify receiverAddress constraints in schema/types/tests ==" fd -i "schema.ts|index.ts|erc4626.validator.test.ts" src | while read -r f; do echo "----- $f -----" rg -n -C2 "receiverAddress|minLength|pattern|format|empty|blank|ActionArguments" "$f" || true done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/validators/evm/erc4626/erc4626.validator.ts` at line 128, The assignment const receiverAddress = args?.receiverAddress || undefined silently treats empty string as absent; change it to explicitly handle blank values by checking args?.receiverAddress for null/undefined and empty string: if args?.receiverAddress === '' either normalize to undefined only after explicit decision or throw a validation error, otherwise set receiverAddress = args.receiverAddress; update the validation path that uses receiverAddress (the variable and any consuming function) so blank strings are rejected or normalized consistently rather than being boolean-coerced away.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/validators/evm/erc4626/erc4626.validator.ts`:
- Line 128: The assignment const receiverAddress = args?.receiverAddress ||
undefined silently treats empty string as absent; change it to explicitly handle
blank values by checking args?.receiverAddress for null/undefined and empty
string: if args?.receiverAddress === '' either normalize to undefined only after
explicit decision or throw a validation error, otherwise set receiverAddress =
args.receiverAddress; update the validation path that uses receiverAddress (the
variable and any consuming function) so blank strings are rejected or normalized
consistently rather than being boolean-coerced away.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8f0e0ca8-d953-43f9-9156-77ffa4ca03f6
📒 Files selected for processing (2)
src/json/schema.tssrc/validators/evm/erc4626/erc4626.validator.ts
✅ Files skipped from review due to trivial changes (1)
- src/json/schema.ts
ajag408
requested review from
0xcacti,
Philippoes,
aditya172926,
apurvmishra,
auralshin,
jdomingos and
raiseerco
and removed request for
apurvmishra
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| } | ||
|
|
||
| const receiverAddress = args?.receiverAddress || undefined; | ||
|
|
There was a problem hiding this comment.
agree with the bot, receiverAddress looks wired correctly e2e, one small follow-up might be adding a runtime type check here as well, since the schema only covers the JSON path and a truthy no string value could still reach .toLowerCase
There was a problem hiding this comment.
Good catch, now using isNonEmptyString(args?.receiverAddress) to guard the extraction, which matches the pattern used by the Tron and Solana validators for their args.validatorAddress access. An empty string or non-string value now normalizes to undefined, falling back to userAddress as expected.
Esp since this is a security library good to have clear errors instead of unhandled TypeErrors, thanks for flagging!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/validators/evm/erc4626/erc4626.validator.ts`:
- Around line 130-133: Fix Prettier formatting around the receiverAddress
declaration: reflow the ternary so indentation matches project Prettier rules
(align the ? and : with the variable declaration) for the const receiverAddress
= isNonEmptyString(args?.receiverAddress) ? args.receiverAddress : undefined;
expression and remove the extra blank line near the top of the file (around the
earlier imports/definitions). Run Prettier or apply the same formatting to the
erc4626.validator.ts file to ensure lint/CI passes, focusing on the
receiverAddress declaration and the stray blank line near the top.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ffdaad19-9dec-4ceb-bc7e-04551c3af53c
📒 Files selected for processing (1)
src/validators/evm/erc4626/erc4626.validator.ts
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores
Fixes two false positive patterns surfaced during the production MONITOR soak:
Receiver address mismatch — Clients like DeBlock pass
receiverAddressin the API args to deposit/withdraw on behalf of users (e.g., a funded USDC wallet deposits into a vault, shares go to the user's wallet). Shield only knew theuserAddressand flagged the different receiver as suspicious. Shield is fully decoupled from the API runtime and couldn't distinguish a legitimatereceiverAddressfrom a malicious fund redirect.USDT zero approval blocked — Ethereum USDT requires
approve(0)beforeapprove(amount)due to its non-standard ERC-20 implementation. Shield rejected zero-amount approvals as invalid.Changes
1.
args.receiverAddress— intent-based receiver validationAdded
receiverAddress?: stringtoActionArguments, allowing callers to declare the intended receiver address. The ERC-4626 validator now usesargs.receiverAddress ?? userAddressas the expected receiver in bothvalidateSupply()andvalidateWithdraw().Behavior:
receiverAddressnot provided, calldata receiver ==userAddressuserAddressreceiverAddressnot provided, calldata receiver !=userAddressuserAddressreceiverAddressprovided, calldata receiver matches itreceiverAddressreceiverAddressprovided, calldata receiver does NOT match itreceiverAddressThis mirrors how the monorepo's
StakeArguments.receiverAddressworks — the same field name, same semantics. The monorepo passes the user's declared receiver, and Shield verifies the transaction calldata matches that intent. Standalone Shield consumers who don't passargs.receiverAddressget identical behavior to before.Note: The
ownercheck invalidateWithdraw()is unchanged — the share owner must always matchuserAddress. Only the asset receiver gets the override.2. Allow zero-amount ERC-20 approvals
Removed the zero-amount approval block in
validateApproval(). A zero approval (approve(spender, 0)) is a revocation — it reduces allowance to zero, making it a strictly defensive operation. The remaining checks (correctapproveselector, spender is a whitelisted vault,tx.tomatches vault input token) are sufficient.3. Dev dependency fix (CI)
Updated
ts-jestto resolve a criticalhandlebarsvulnerability (GHSA-2w6w-674q-4c4q) in the transitive dependency chain (ts-jest > handlebars). Dev-only — does not affect the published package. Was causingpnpm auditto fail in CI.Affected soak yields (now resolved)
base-usdc-steakusdc-0xbeef0e...-4626-vault(Morpho/Steakhouse)base-usdc-smusdc-0x616a4e...-4626-vault(Seamless)ethereum-usdt-gtusdtp-0xf3557ad...-4626-vault(Morpho/Gauntlet)approve(0)beforeapprove(amount)Files changed
src/types/index.tsreceiverAddress?: stringtoActionArgumentssrc/validators/evm/erc4626/erc4626.validator.tsargs.receiverAddress; thread tovalidateSupply()+validateWithdraw(); update receiver checks to usereceiverAddress ?? userAddress; remove zero-approval block invalidateApproval()src/validators/evm/erc4626/erc4626.validator.test.tsreceiverAddresstests (SUPPLY: match, mismatch, default with/without override; WITHDRAW: match, mismatch); update zero-approval test to expectisValid: truesrc/json/schema.tsreceiverAddresstoargsschema (required due toadditionalProperties: false)package.jsonpnpm-lock.yamlts-jestto resolve criticalhandlebarsvulnerability (CI fix)Test plan
receiverAddressoverride tests pass:args.receiverAddressmatching calldata receiver → Safeargs.receiverAddressNOT matching calldata receiver → Blockedargs.receiverAddress, receiver != user → Blocked (default behavior preserved)args.receiverAddress, receiver == user → Safe (default behavior preserved)args.receiverAddressmatching calldata receiver → Safeargs.receiverAddressNOT matching calldata receiver → Blockedpnpm testpnpm lintBackward compatibility
Fully backward-compatible.
args.receiverAddressis optional — consumers who don't provide it get identical validation behavior to the previous version. The zero-approval change is a relaxation (previously blocked, now allowed), which cannot break existing valid flows.Downstream dependency
The monorepo (PR 4) will bump
@yieldxyz/shieldto this version and wireargs.receiverAddressthrough to Shield at the call sites inshield-validation.service.ts.