Roundrip fee on proportional remove #1010
Conversation
| @@ -872,6 +873,13 @@ contract Vault is IVaultMain, VaultCommon, Proxy { | |||
| _totalSupply(params.pool), | |||
| bptAmountIn | |||
| ); | |||
|
|
|||
| if (_addLiquidityCalled().tGet(params.pool)) { | |||
There was a problem hiding this comment.
Note: if we clear this flag here, it's easy to bypass.
You add a huge amount, make a decoy proportional exit, and then a big exit.
Whenever the flag is set, it'll be set for the rest of the tx. A more correct approach would be to clean it when transient modifier ends, but we would need a transient set for that and I really don't think it's worth the added complexity.
There was a problem hiding this comment.
Agree that's probably not worth it for an edge case. Although... maybe aggregators might bundle lots of transactions and come across that, since it just checks the pool, not the pool + user, so this could also get triggered on bundled transactions with multiple unrelated users doing liquidity operations on a popular pool. Maybe worth investigating if that's possible? If one person added and another person removed, the second one would get unexpectedly taxed.
Though even storing the user wouldn't be foolproof, as someone could craft a transaction where the add and remove steps were done from separate addresses controlled by the same account.
jubeira
requested review from
EndymionJkb,
joaobrunoah and
elshan-eth
and removed request for
EndymionJkb and
joaobrunoah
| * @notice Returns `true` if `addLiquidity` was called for the given pool in this transaction. | ||
| * @param pool Address of the pool to check. |
There was a problem hiding this comment.
| * @notice Returns `true` if `addLiquidity` was called for the given pool in this transaction. | |
| * @param pool Address of the pool to check. | |
| * @notice This flag is used to detect and tax "round trip" transactions (adding/removing liquidity in the same pool) | |
| * @param pool Address of the pool to check | |
| * @return liquidityAdded True if liquidity has been added to this pool in the current transaction |
There was a problem hiding this comment.
Could also add a dev section here with more detail on why this is desirable / improves security, and documenting limitations.
| @@ -872,6 +873,13 @@ contract Vault is IVaultMain, VaultCommon, Proxy { | |||
| _totalSupply(params.pool), | |||
| bptAmountIn | |||
| ); | |||
|
|
|||
| if (_addLiquidityCalled().tGet(params.pool)) { | |||
There was a problem hiding this comment.
Agree that's probably not worth it for an edge case. Although... maybe aggregators might bundle lots of transactions and come across that, since it just checks the pool, not the pool + user, so this could also get triggered on bundled transactions with multiple unrelated users doing liquidity operations on a popular pool. Maybe worth investigating if that's possible? If one person added and another person removed, the second one would get unexpectedly taxed.
Though even storing the user wouldn't be foolproof, as someone could craft a transaction where the add and remove steps were done from separate addresses controlled by the same account.
Description
Proposal: tax proportional exits whenever liquidity was added in the same transaction.
Every other remove kind charges fees already as it's unbalanced, and there's no way to remove with more than one token close to the existing proportion to avoid fees (unlike add liquidity unbalanced).
This should make roundrips more difficult, and make swap sandwiches economically unfeasible.
For now I'll keep the liquidity approximation tests as they are, effectively clearing up the flag so as not to apply the tax when calling
removeLiquidityProportional. In practice, the fee will make indirect swap paths worse than our current fuzz tests, and that's a desirable outcome.Type of change
Checklist:
main, or there's a description of how to mergeIssue Resolution
N/A