Submitted by osmanozdemir1, also found by minhtrng (1, 2), fnanni (1, 2), 0xloscar01, KupiaSec, ether_sky, hash, 0xDING99YA, and SpicyMeatball
The positions in this protocol are ERC1155 tokens and they can be minted or burned.
Token transfers are extremely limited in the protocol:
Users current liquidity in their positions is tracked with a storage variable called s_accountLiquidity. This mapping is overwritten during transfers and the whole value is transferred. The reason for not allowing partial transfers is that partial transfers will mess up the whole storage updating mechanism.
The requirements mentioned above are checked here:
The check related to whether all balance is transferred or not is made by checking the right slot of the senders liquidity using fromLiq.rightSlot(). Right now, I want to point out there is no check related to the left slot. Ill get there later.
Now, we have to understand how position keys are constructed, and how the left slot and right slot work. Lets start with the position keys:
They are constructed with pool address, user address, token type, lower tick and upper tick. The most important thing I want to mention here is that whether the position is long or short is not in the position key. The thing that matters is the token type (put or call). Which means:
Short put and Long put orders have the same position key (for the same tick range) but different token IDs.
The second thing we need to know is the left and right slot mechanism:
The left slot holds the removed liquidity values and the right slot holds the net liquidity values.
These values are updated in the _createLegInAMM during minting and burning depending on whether the action is short or long or mint or burn etc.
As I mentioned above, only the right slot is checked during transfers. If a user mints a short put (deposits tokens), and then mints a long put (withdraws tokens) in the same ticks, the right slot will be a very small number but the user will have two different ERC1155 tokens (token Ids are different for short and long positions, but position key is the same). Then that user can transfer just the partial amount of short put tokens that correspond to the right slot.
Ill provide two different scenarios here where the sender is malicious in one of them, and a naive user in another one. You can also find a coded PoC below that shows all of these scenarios.
Scenario 1: Alice(sender) is a malicious user
There are two big problems here:
Now imagine a malicious user minting a huge amount of short put (deposit), minting 99% of that amount of long put (withdraw), and transferring that 1% to the victim. Its basically setting traps for the victim by transferring tiny amount of net liquidities. The victims account looks like he removed a lot of liquidity, and if the victim mints positions in the same range in the future, the owed premiums for this position will be extremely different, and much higher than it should be.
Scenario 2: Alice (sender) is a naive user
The initial steps are the same as those above. Alice is just a regular user.
SemiFungiblePositionManager.sol#L961C9-L980C18
Alices account liquidity storage was updated before (step 4) and removedLiquidity was 0. After burning her long put tokens, the new removedLiquidity (L979 above) will be an enormous number since it is inside the unchecked block.
Down below you can find a coded PoC that proves all scenarios explained above. You can use the protocols own setup to test this issue:
The result after running the test:
Foundry
At the moment, the protocol checks if the whole net liquidity is transferred by checking the right slot. However, this restriction is not enough and the situation of the left slot is not checked at all.
The transfer restriction should be widened and users should not be able to transfer if their removed liquidity (left slot) is greater than zero.
Token-Transfer
dyedm1 (Panoptic) confirmed
