Skip to content

fix(swap): use BetterSwap AggregatorRouter directly - #662

Open
Emilvooo wants to merge 1 commit into
vechain:mainfrom
Emilvooo:fix/betterswap-onchain-router
Open

Emilvooo wants to merge 1 commit into
vechain:mainfrom
Emilvooo:fix/betterswap-onchain-router

Conversation

@Emilvooo

@Emilvooo Emilvooo commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Use BetterSwap’s AggregatorRouter directly for on-chain quotes and swaps.
  • Update the router address and native VET handling.
  • Fix zero and fractional slippage handling.
  • Keep VeTrade available alongside BetterSwap.

Validation
Build and ESLint pass. Three live mainnet simulations passed without signing or broadcasting transactions.

Summary by CodeRabbit

  • New Features
    • BetterSwap quotes and swaps now use on-chain routing, with quotes that include router fees and exact-amount token approvals.
    • Swap simulation checks that token flows match the expected result.
  • Bug Fixes
    • Slippage settings are validated and rounded to whole basis points; invalid settings return an empty quote.
  • Limitations
    • BetterSwap supports mainnet only and rejects swaps involving wrapped VET. Price impact is not estimated.

@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown
Contributor

👋 Thanks for your contribution!

Since this PR comes from a forked repository, the lint and build will only run for internal PRs for security reasons.
Please ensure that your PR is coming from a meaningful branch name. Eg. feature/my-feature not main

      **Next steps:**
      1. A maintainer will review your code
      2. If approved, they'll add the `safe-to-build` label to trigger build and test
      3. **After each new commit**, the maintainer will need to remove and re-add the label for security

Thank you for your patience! 🙏

@coderabbitai

coderabbitai Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: defaults
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: a0368454-e04d-4a83-a321-1c7053b8a4f4
📥 Commits

Reviewing files that changed from the base of the PR and between a560dda and 686652c.

📒 Files selected for processing (3)
  • packages/vechain-kit/src/utils/swap/README.md
  • packages/vechain-kit/src/utils/swap/betterSwap.tsx
  • packages/vechain-kit/src/utils/swap/uniswapV2Aggregator.ts

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

BetterSwap now uses the Uniswap V2-compatible aggregator for on-chain quotes and swap transaction construction. The implementation validates network and token addresses. The shared quote method validates slippage tolerance and rounds its basis-point calculation.

Changes

BetterSwap on-chain aggregation

Layer / File(s) Summary
Shared quote slippage validation
packages/vechain-kit/src/utils/swap/uniswapV2Aggregator.ts
The quote method preserves explicit 0% tolerance, rejects non-finite values and percentages outside 0–100, and rounds the percentage when calculating the basis-point multiplier. Invalid values follow the existing error path and return an empty quote.
BetterSwap adapter and documentation
packages/vechain-kit/src/utils/swap/betterSwap.tsx, packages/vechain-kit/src/utils/swap/README.md
BetterSwap configures the shared aggregator with its router and native-token placeholder. It rejects non-mainnet requests and explicit wrapped-VET tokens, delegates quote and transaction construction, and clears priceImpact from successful quotes. The documentation describes the on-chain route and its constraints.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant BetterSwap
  participant UniswapV2Aggregator
  participant AggregatorRouter
  Caller->>BetterSwap: Request quote
  BetterSwap->>UniswapV2Aggregator: Validate and request quote
  UniswapV2Aggregator->>AggregatorRouter: Call getAmountsOut
  AggregatorRouter-->>UniswapV2Aggregator: Return output amounts
  UniswapV2Aggregator-->>BetterSwap: Return quote
  BetterSwap-->>Caller: Return quote without price impact
Loading

Merge Risk: ⚪ Minimal · up to 68665

BetterSwap now quotes and builds swaps through the on-chain router, and slippage validation is stricter. No concrete merge-blocking issue was identified.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 68665

The fixed router reduces dependence on executable data returned by an external API. However, BetterSwap no longer checks that execution matches the original quote’s parameters or expiry. Normal interface checks reduce exposure, but public callers can still combine stale or mismatched quotes with new transaction parameters. A wallet signature remains required.

Retained concerns

  • Medium · security · observed: Delegation removes BetterSwap’s execution-time quote binding and expiry checks. The builder now takes the path and minimum output from a quote while taking the spend amount and recipient from current parameters, and renews the transaction deadline. Unlike the previous implementation, it can construct clauses for a reused or mismatched quote instead of rejecting it, weakening the boundary between quoted intent and signed asset movement.
Security review details

Security Blast Radius

  • inferred — The material exposure is signed asset movement from an individual wallet through the fixed BetterSwap router. Native value and new ERC-20 approvals use the current input amount. Under the router’s expected swap semantics, an unbound ERC-20 path could also select a different token already approved to that router. This is conditional on accepting the mismatched clauses and signing; quote data cannot replace the configured router.

Security Findings and Attack Paths

  • inferred — A caller that reuses a smaller-amount quote with a larger current input can retain the smaller quote’s minimum-output floor while spending the larger amount. A mismatched path can likewise differ from the selected output asset. The previous BetterSwap execution validation rejected amount and endpoint mismatches. Exploitation would require influence over application-supplied parameters or quote selection and a resulting wallet signature; an unauthenticated remote attack path was not demonstrated.

Trust Boundaries and Controls

  • observed — Important countercontrols remain: the router and spender are fixed, quote retrieval is keyed by current parameters and refreshes periodically, simulation verifies expected token flows, and the included swap interface derives its recipient from the connected account. These reduce normal-use mismatches but do not restore execution-time quote binding for public callers.

Resilience and Maintainability Implications

  • observed — BetterSwap previously rejected expired quotes during construction. The delegated builder instead generates a deadline twenty minutes after each build, so repeated construction extends transaction eligibility without checking the original quote’s age. This changes the security-relevant recovery behavior for retained quotes.

Hardening Proposals

  • proposed — Preserve the shared implementation while binding quotes to their original input amount, token endpoints, recipient, slippage policy and validity interval. Validate that binding before constructing signing clauses, and require a new quote when the bound intent changes or expires.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: using BetterSwap’s AggregatorRouter directly for swaps.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant