Skip to content

fix: align approval hooks and permit spender checks#7620

Closed
fairlighteth wants to merge 6 commits into
developfrom
deepsec/medium-04-approval-permit-integrity
Closed

fix: align approval hooks and permit spender checks#7620
fairlighteth wants to merge 6 commits into
developfrom
deepsec/medium-04-approval-permit-integrity

Conversation

@fairlighteth

@fairlighteth fairlighteth commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Addresses the approval/permit integrity findings in cowswap-frontend by making spender handling, widget approval gating, and permit cache keys explicit across the approval paths.

This also includes follow-up hardening found during review:

  • unresolved allowance reads now keep approval required instead of treating approval as unnecessary;
  • permit detection no longer caches transient errors;
  • locally cached token+spender permit support still maps back to token-address UI tags;
  • invalid custom permit spenders are ignored until they validate as addresses;
  • DAI-like permit approval semantics are reported conservatively to the widget hook;
  • ETH-flow ignores stale partial approval amounts for another token;
  • TWAP creation waits for the zero-approval decision before building creation txs;
  • object-shaped custom ERC-20s are not classified as native only by symbol.

Review guide

  1. Shared approval gate/helper

    • Start with callOnBeforeApprovalWidgetHook; it centralizes the widget ON_BEFORE_APPROVAL payload and keeps the actual spender/approval amount explicit.
  2. Classic + permit approval paths

    • Review classic approval state/callback changes, useNeedsApproval, permit-in-advance, and permit info lookup. These paths should now use the same spender for allowance, pending approval, hook payload, and permit cache lookups.
  3. ETH-flow + Safe bundle paths

    • Review ETH-flow approval amount selection, Safe approval bundles, and Safe ETH bundles. These paths should go through the same approval gate before approval tx construction/submission.
  4. Permit cache/storage changes

    • Review spender-aware cache keys, transient-error eviction, zero-amount permit handling, and local permit-compatible token mapping from token+spender cache keys back to token addresses.

QA Testing

Automated checks passed on head e579f425f:

  • pnpm exec jest --config apps/cowswap-frontend/jest.config.mjs --runInBand --runTestsByPath apps/cowswap-frontend/src/modules/ethFlow/containers/EthFlow/utils/getEthFlowCurrencyToApprove.test.ts apps/cowswap-frontend/src/common/hooks/useNeedsApproval.test.ts apps/cowswap-frontend/src/modules/erc20Approve/hooks/useApproveCurrency.test.ts apps/cowswap-frontend/src/modules/erc20Approve/hooks/useGeneratePermitInAdvanceToTrade.test.ts apps/cowswap-frontend/src/modules/permit/hooks/usePermitInfo.test.ts apps/cowswap-frontend/src/modules/permit/hooks/usePermitCompatibleTokens.test.ts
    • 6 suites passed, 43 tests passed.
  • pnpm exec nx run permit-utils:test --runInBand --runTestsByPath=libs/permit-utils/src/lib/generatePermitHook.test.ts --runTestsByPath=libs/permit-utils/src/lib/getTokenPermitInfo.test.ts
    • 2 suites passed, 6 tests passed.
  • pnpm exec nx run common-utils:test --runInBand --runTestsByPath=libs/common-utils/src/getIsNativeToken.test.ts
    • Command passed; Nx/Jest ran the common-utils suite set from cache on the final run.
  • pnpm exec tsc --noEmit -p apps/cowswap-frontend/tsconfig.app.json
  • pnpm exec tsc --noEmit -p libs/permit-utils/tsconfig.lib.json
  • pnpm exec tsc --noEmit -p libs/common-utils/tsconfig.lib.json

GitHub checks for e579f425f were still running when this body was updated.

Known QA gaps

  • No fresh real-wallet signing pass was done on the latest head for ERC-20 approval, EIP-2612 permit, or DAI-like permit signing.
  • No manual Safe bundle approval/rejection coverage was done on the latest head.
  • No manual TWAP creation run was done for a token that requires zero-approval before changing allowance.

Preview URLs

Surface URL
swap-dev https://swap-dev-git-deepsec-medium-04-approval-perm-eb8761-cowswap-dev.vercel.app
cowfi https://cowfi-git-deepsec-medium-04-approval-permit-integrity-cowswap.vercel.app
explorer-dev https://explorer-dev-git-deepsec-medium-04-approval-0f08b3-cowswap-dev.vercel.app
storybook https://storybook-git-deepsec-medium-04-approval-per-811f70-cowswap-dev.vercel.app
widget-configurator https://widget-configurator-git-deepsec-medium-04-ap-f74059-cowswap-dev.vercel.app
sdk-tools https://sdk-tools-git-deepsec-medium-04-approval-per-727567-cowswap-dev.vercel.app

@vercel

vercel Bot commented Jun 6, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cowfi Ready Ready Preview Jul 9, 2026 4:17pm
explorer-dev Ready Ready Preview Jul 9, 2026 4:17pm
storybook Ready Ready Preview Jul 9, 2026 4:17pm
swap-dev Ready Ready Preview Jul 9, 2026 4:17pm
widget-configurator Ready Ready Preview Jul 9, 2026 4:17pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
cosmos Ignored Ignored Jul 9, 2026 4:17pm
sdk-tools Ignored Ignored Preview Jul 9, 2026 4:17pm

Request Review

@coderabbitai

coderabbitai Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 76164adf-d212-401f-9b47-c3f2e82a6118

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch deepsec/medium-04-approval-permit-integrity

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jun 6, 2026

Copy link
Copy Markdown

Deploying explorer-dev with  Cloudflare Pages  Cloudflare Pages

Latest commit: e579f42
Status: ✅  Deploy successful!
Preview URL: https://791de3b3.explorer-dev-dxz.pages.dev
Branch Preview URL: https://deepsec-medium-04-approval-p.explorer-dev-dxz.pages.dev

View logs

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jun 6, 2026

Copy link
Copy Markdown

Deploying swap-dev with  Cloudflare Pages  Cloudflare Pages

Latest commit: e579f42
Status: ✅  Deploy successful!
Preview URL: https://0c0e309b.swap-dev-5u6.pages.dev
Branch Preview URL: https://deepsec-medium-04-approval-p.swap-dev-5u6.pages.dev

View logs

- key permit support caches by spender
- bypass default pre-generated permit data for custom spenders
- restore eth-flow approval typing so app checks pass

@fairlighteth fairlighteth left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AI Review (Codex GPT-5, worked 4m): no new non-duplicate findings

Review completed. I found no new non-duplicate comments worth posting.

Review scope and related context

Related context checked:

  • usePermitInfo, permittableTokensAtom, and getTokenPermitInfo: current code now keys permit support lookup/cache by spender and avoids reusing default pre-generated permit data for custom spenders.
  • Approval-gate call sites in classic approval, permit pre-generation, ETH-flow, and Safe bundle paths: current diff looks internally consistent with the updated PR description.
  • Existing PR comments: only bot/deploy comments are present, so there were no active human review threads to de-duplicate against.

@fairlighteth

Copy link
Copy Markdown
Contributor Author
PR - Automated QA Scope Pass (Codex GPT-5, worked 45m): hooks composer + spender-cache behavior verified

Outcome

  • ✅ Passed: disconnected hooks composer verified on the PR runtime path in Chromium and Firefox
  • ✅ Passed: wallet-connected-ui Hook Store flow verified in Chromium on Sepolia with a mocked injected EIP-1193 provider
  • ✅ Passed: spender-aware permit-cache logic verified on the PR build with a deterministic browser fixture
  • ✅ Scope note: this pass covered the hooks route, Hook Store shell, and spender-cache runtime path; intentionally untested QA items are listed below
  • ⚠️ Follow-up note: Permit a token was not surfaced in Hook Store UI, so that path was verified via browser harness rather than direct form interaction

At a glance

  • Source: local Playwright run against PR head 3567a3c3e2ca4d50546b0af8051339067ee97fc3
  • Browsers: Chromium 140.0.7339.16 and Firefox 141.0
  • OS: Linux
  • Wallet states:
    • disconnected: yes
    • provider-injected: yes
    • wallet-connected-ui: yes
    • signing-capable: no

Source under test

  • Browser-based verification was performed locally with Playwright against the local worktree at the PR head SHA.
  • A PR Vercel preview was checked first, but the final verification used the local app because the interactive trade surface was not reliably rendered there in headless mode.
  • Results were visually inspected from captured screenshots.

🔁 Re-run

  • Frontend: pnpm start:cowswap
  • Routes:
    • http://127.0.0.1:3004/#/1/swap/hooks
    • http://127.0.0.1:3004/#/11155111/swap/hooks
  • Local state required:
    • localStorage["redux_localstorage_simple_user"] = {"hooksEnabled":true,"matchesDarkMode":false,"userDarkMode":null,"userLocale":null}
  • Fixtures/mocks:
    • mocked injected EIP-1193 provider for the wallet-connected-ui Sepolia pass
    • deterministic browser harness against https://ethereum-rpc.publicnode.com for the spender-cache proof

Expected results:

  1. Open the hooks route and click Swap now
    Expected: the hooks composer loads with Add Pre-Hook Action and Add Post-Hook Action
  2. On Sepolia with the mocked provider, open Add Pre-Hook Action
    Expected: Hook Store opens in wallet-connected-ui state and renders the hook list modal cleanly
  3. Run the deterministic spender-cache harness on the PR build
    Expected: same-spender checks stay deduped, while different spenders trigger separate permit checks for the same token

🧪 Browser coverage

  • Chromium 140.0.7339.16
    • disconnected hooks composer
    • wallet-connected-ui Hook Store on Sepolia
    • ✅ deterministic spender-cache proof
  • Firefox 141.0
    • disconnected hooks composer

Why these adjacent flows were included:

  • The hooks composer route shares the same trade container and hook-entry shell touched by the spender-aware permit changes.
  • The Sepolia Hook Store pass exercises the wallet-aware modal/container path adjacent to the PermitHookApp consumer even though the permit dapp itself was not surfaced.
  • The deterministic browser harness directly exercises the shared permit-utils request-cache boundary changed by the PR, which is the critical stale-spender failure mode.

🖼️ Artifacts
Chromium disconnected hooks composer
Chromium disconnected hooks composer

Chromium wallet-connected-ui Hook Store on Sepolia
Chromium wallet-connected Hook Store on Sepolia

Firefox disconnected hooks composer
Firefox disconnected hooks composer

Note: Permit hook UI was not directly reachable

Impact:

  • This does not materially undercut the PR claim because the changed spender-cache logic was still exercised directly in-browser on the PR build.

Observed behavior:

  • Permit a token did not appear in the Hook Store lists on the tested mainnet or Sepolia routes.
  • The broader hooks route and Hook Store UI still rendered correctly.

Interpretation:

  • This looks separate from the spender-aware cache change itself.
QA Scope Not Covered (2)
  • PermitHookApp itself was not directly visible in Hook Store UI during this run
  • No signing-capable approval, signature, or order-submission flow was exercised
Commands + Setup

Commands run:

  • pnpm start:cowswap
  • PLAYWRIGHT_BROWSERS_PATH=$HOME/.cache/ms-playwright-local node - <<'NODE' ... NODE for the Chromium disconnected hooks-composer pass
  • PLAYWRIGHT_BROWSERS_PATH=$HOME/.cache/ms-playwright-local node - <<'NODE' ... NODE for the Chromium mocked-provider wallet-connected-ui Sepolia Hook Store pass
  • PLAYWRIGHT_BROWSERS_PATH=$HOME/.cache/ms-playwright-local node - <<'NODE' ... NODE for the Chromium browser-executed deterministic permit-cache harness against mainnet RPC
  • PLAYWRIGHT_BROWSERS_PATH=$HOME/.cache/ms-playwright-local node - <<'NODE' ... NODE for the Firefox disconnected hooks-composer smoke pass

Environment normalization used:

  • Playwright locale normalized to en-US
  • Chromium launched with --lang=en-US

Still manual if desired:

  • Human UX review of Hook Store availability/filtering for Permit a token
  • A true signing-capable approval or swap submission with a real wallet/session

Residual gaps:

  • The actual PermitHookApp form remains unproven through direct UI interaction in this run.
  • No real-wallet signing path was exercised.

…approval-permit-integrity

# Conflicts:
#	apps/cowswap-frontend/src/modules/tradeFlow/services/safeBundleFlow/safeBundleEthFlow.ts

Copy link
Copy Markdown
Contributor Author

@github-actions github-actions Bot locked and limited conversation to collaborators Jul 10, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant