Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CL][Swap Router Module]: investigate reducing the need for making public swap API with swapFee as parameter #3130

Closed
p0mvn opened this issue Oct 24, 2022 · 4 comments
Assignees
Labels
C:x/concentrated-liquidity C:x/gamm Changes, features and bugs related to the gamm module. C:x/swap-router F: concentrated-liquidity Tracking the development of concentrated liquidity feature to improve filtering on the project board T:task ⚙️ A task belongs to a story

Comments

@p0mvn
Copy link
Member

p0mvn commented Oct 24, 2022

Background

In #3128 we refactored gamm to lift multihop logic up to swap-router. This has led to
exporting a gamm and concentrated-liquidity swap APIs that take swap fee as parameter.

This poses a security risk since a developer might accidentally provide a fee that is lower than the actual pool fee.

Ultimately, the only reason for exposing swap fee in the API is so that we can reduce the fee for osmo routed mutlihop:

swapFeeMultiplier = gammtypes.MultihopSwapFeeMultiplierForOsmoPools.Clone()
}

Suggested Design

We should investigate potential approaches to mitigate this security vulnerability and come up with an abstraction that does not expose swapFee publicly.

Acceptance Criteria

  • refactor to avoid exposing swapFee in SwapExactAmountIn globally
  • refactor to avoid exposing swapFee in SwapExactAmountOut globally
  • refactor tests
  • add new if applicable
@p0mvn p0mvn added C:x/gamm Changes, features and bugs related to the gamm module. C:x/concentrated-liquidity C:x/swap-router F: concentrated-liquidity Tracking the development of concentrated liquidity feature to improve filtering on the project board labels Oct 24, 2022
@p0mvn p0mvn moved this to Needs Review 🔍 in Osmosis Chain Development Oct 24, 2022
@p0mvn p0mvn added the T:task ⚙️ A task belongs to a story label Oct 24, 2022
@p0mvn p0mvn removed this from the CL7: Swap Router V14 QA and Tweaks milestone Jan 9, 2023
@p0mvn p0mvn removed their assignment Jan 12, 2023
@p0mvn p0mvn added this to the CL 8: Fees and Misc Bug Fixes milestone Jan 13, 2023
@p0mvn p0mvn moved this from Needs Review 🔍 to Todo 🕒 in Osmosis Chain Development Jan 13, 2023
@mattverse mattverse self-assigned this Jan 16, 2023
@mattverse
Copy link
Member

@p0mvn Regarding this issue, it seems inevitable to completely remove swapFee related argument since we're handling multihop logic in the pool model layer. It seems that there are two possible ways to tackle this

  1. move multihop related logic to each pool module layer
  2. refactor so that we don't pass in the swapFee itself but change the swap Fee into a boolean argument names isMultihopRouted and then calculate the actual swap fee within each pool module's swap methods.

Although option 2 does not completely solve the problem this issue is particularly mentioning, it does reduce the security risk introduced in this issue. What are your thoughts?

@mattverse
Copy link
Member

Decision made to take option2 above. If code duplication seems unnecessarily excessive, dropping this issue since it would not make sense to make the trade off of massive code duplication vs minor API vulnerability

@mattverse
Copy link
Member

Blocked on investigation on how multihop fees are being charged right now

@mattverse
Copy link
Member

After some investigation, I came to the conclusion of having to leave swapFees as is.

Currently we're taking the multihop swap fees via max of individual swap fees and the average swap fees of the swaps). This means that we always need to calculate the swap fees at the highest level that multihop takes place and then provide them via parameters. This lead to the conclusion that our current API stands valid, open to any other ideas or suggestions

@p0mvn p0mvn closed this as completed Feb 27, 2023
@github-project-automation github-project-automation bot moved this from Todo 🕒 to Done ✅ in Osmosis Chain Development Feb 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/concentrated-liquidity C:x/gamm Changes, features and bugs related to the gamm module. C:x/swap-router F: concentrated-liquidity Tracking the development of concentrated liquidity feature to improve filtering on the project board T:task ⚙️ A task belongs to a story
Projects
Archived in project
Development

No branches or pull requests

2 participants