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

stableswap: Add check for correct input asset denoms to joinPoolSharesInternal and updatePoolForJoin #2266

Closed
AlpinYukseloglu opened this issue Aug 1, 2022 · 0 comments · Fixed by #2301
Assignees
Labels
C:x/gamm Changes, features and bugs related to the gamm module. Good first issue T:bug 🐛 Something isn't working

Comments

@AlpinYukseloglu
Copy link
Contributor

AlpinYukseloglu commented Aug 1, 2022

Background

Our current implementation of JoinPool for stableswap checks the number of input assets, but does not check what their specific denoms are. This allows for a scenario where arbitrary tokens can be sent into a pool, much like was the case in #1906.

However, unlike in #1906, our stableswap implementation has no implicit denom check when updating pool assets, so anything that is passed in is just directly added to the pool:

func (p *Pool) updatePoolForJoin(tokensIn sdk.Coins, newShares sdk.Int) {
p.PoolLiquidity = p.PoolLiquidity.Add(tokensIn...)
p.TotalShares.Amount = p.TotalShares.Amount.Add(newShares)
}

If it weren't for a division by zero here incidentally catching denom deposits for denoms not in the pool, this would be a critical bug as it would allow spam/poisoned tokens to be sent into a pool to crowd out and ultimately drain liquidity.

We should add a denom check both in joinPoolSharesInternal and, to cover broader future cases, also add a check to updatePoolForJoin to make sure joining a pool does not change the number of assets in it.

Suggested Design

  • Expand the condition below to ensure that p.GetTotalPoolLiquidity() is a DenomsSubsetOf of TokensIn and vice versa (i.e. make sure denoms are equivalent):

    } else if len(tokensIn) != p.NumAssets() {

  • Add a length check similar to the following to updatePoolForJoin in stableswap/pool.go:

    if len(p.PoolLiquidity) != numTokens {
    panic("updatePoolLiquidityForSwap changed number of tokens in pool")
    }

Acceptance Criteria

  • Existing and new tests pass
@AlpinYukseloglu AlpinYukseloglu added T:bug 🐛 Something isn't working C:x/gamm Changes, features and bugs related to the gamm module. Good first issue labels Aug 1, 2022
@osmo-bot osmo-bot moved this to Needs Review 🔍 in Osmosis Chain Development Aug 1, 2022
@hieuvubk hieuvubk self-assigned this Aug 4, 2022
Repository owner moved this from Needs Review 🔍 to Done ✅ in Osmosis Chain Development Aug 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:x/gamm Changes, features and bugs related to the gamm module. Good first issue T:bug 🐛 Something isn't working
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants