iqlusioninc / liquidity-staking-module

Other
87 stars 29 forks source link

CheckExceedsGlobalLiquidStakingCap bug #89

Closed xlab closed 1 year ago

xlab commented 1 year ago

Hello, just discovered yesterday that CheckExceedsGlobalLiquidStakingCap does some share computations wrongly on a simple state with 1 validator and 1 delegator, 0 tokenized. Example below

Setup

liquidstakingd init test --chain-id test-core-1
echo $test_mnemonic | liquidstakingd keys add test --recover --keyring-backend test
liquidstakingd add-genesis-account test 100000000000000stake --keyring-backend test
liquidstakingd gentx test 10000000stake --chain-id test-core-1 --keyring-backend test
liquidstakingd collect-gents

Last part of the setup is to patch genesis.json and set "global_liquid_staking_cap": "0.500000000000000000"

Tokenize

liquidstakingd tx staking tokenize-share xxx 10000000stake xxx --from test -y --gas auto --gas-adjustment 1.2 --keyring-backend=test --chain-id=test-core-1

So, even with global cap set at 50%, tokenization of 100% of my bond succeeds. After checking with source, the following two lines https://github.com/iqlusioninc/liquidity-staking-module/blob/4062d6bfb6d1fbb1172fc0d635a828869d4d08d1/x/staking/keeper/liquid_stake.go#L64-L65 add tokens to both liquid stake and bonded pool, assuming that the stake is "new" to the chain. Which is fine for IBC-based staking, but in the case above, stake was already bonded. So instead of checking X against X, it checks X against 2*X and succeeds.

I've added additional test cases into TestCheckExceedsGlobalLiquidStakingCap so the test fails now: https://github.com/iqlusioninc/liquidity-staking-module/pull/88

Not sure how to fix it, without breaking IBC staking cap. Maybe we need a different kind of check for different source?

sampocs commented 1 year ago

great catch! I think we can just pass a bool to this function and if it's for a tokenization, then it doesn't double count it

sampocs commented 1 year ago

@xlab added to the fix to #108