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

Transferring 0 amounts of non-existent tokens when all liquidity is removed results in unexpected behaviors #498

Closed
c4-bot-9 opened this issue Dec 11, 2023 · 4 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-256 satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@c4-bot-9
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/tokens/ERC1155Minimal.sol#L90-L118
https://github.com/code-423n4/2023-11-panoptic/blob/aa86461c9d6e60ef75ed5a1fe36a748b952c8666/contracts/SemiFungiblePositionManager.sol#L586-L630

Vulnerability details

Impact

Currently, ERC1155Minimal.sol allows 0 transfers, which means that tokens that were not minted can be "transferred". Normally this wouldn't be a problem, but in registerTokenTransfer a 0 transfer can actually transfer non-zero liquidity due to the possibility of different token ids sharing the same liquidity position key.

This could lead to unexpected scenarios in which liquidity gets trapped or removedLiquidity underflows, affecting the calculation of premiums and fees.

Proof of Concept

Here is a simple test example that exploits the mentioned behavior to reach an unexpected state https://gist.github.com/fnanni-0/8ebb660b1d4ef518a409e2b82b293113.
Running forge test -vv --match-test test_Success_ZeroTransfers outputs a step by step explanation of what is happening:

Alice mints 1010 of token C (short put, i.e. adds liquidity to AMM).

  Minted positionSize: 1010
  Alice balance token C: 1010
  Added liquidity: 50
  Removed liquidity: 0
  

Alice mints 1010 of token A (long put, i.e. removes all liquidity from AMM). Token A and Token C have the same position key.

  Minted positionSize: 1010
  Alice balance token A: 1010
  Added liquidity: 0
  Removed liquidity: 50
  

Zero transfer of non-existent token B to Bob. Alice is left with active positions (1010 of token C and 1010 of token A) but liquidity is now empty.

  Alice balance token C: 1010
  Alice balance token B: 0
  Alice balance token A: 1010
  Bob balance token C: 0
  Bob balance token B: 0
  Bob balance token A: 0
  Added liquidity: 0
  Removed liquidity: 0
  Bob's added liquidity: 0
  Bob's removed liquidity: 50
  

Alice burns 210 of token A --> removed liquidity underflows.

  Alice balance token A: 800
  Alice balance token C: 1010
  Added liquidity: 10
  Removed liquidity: 340282366920938463463374607431768211446

Tools Used

Code inspection.

Recommended Mitigation Steps

Block transfers of non-existent tokens (i.e. block transfers with zero amounts).

Assessed type

Token-Transfer

@c4-bot-9 c4-bot-9 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Dec 11, 2023
c4-bot-9 added a commit that referenced this issue Dec 11, 2023
@dyedm1
Copy link
Member

dyedm1 commented Dec 18, 2023

dup #256

@c4-sponsor c4-sponsor added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Dec 18, 2023
@c4-sponsor
Copy link

dyedm1 (sponsor) confirmed

@c4-judge
Copy link
Contributor

Picodes marked the issue as duplicate of #256

@c4-judge
Copy link
Contributor

Picodes marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate-256 satisfactory satisfies C4 submission criteria; eligible for awards sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

4 participants