QuantAMM

Summary

The UpliftOnlyExample contract incorrectly handles self-transfers of LP NFT positions by deleting the transferred position data, which can lead to a denial of service when attempting to remove liquidity due to division by zero. As well as permanently locking the users funds in the protocol since a valid position is "popped" from the users account.

Vulnerability Details

In the afterUpdate function of UpliftOnlyExample.sol, when an LP NFT is transferred to the same address (self-transfer), the code improperly handles the position data:

Because feeDataArrayLength is not updated when the poolsFeeData array "push's" a new position to the array the feeDataArrayLength will be incorrect and the loop will not work as expected. This is because the to address in this case is the same as the from address. So by pushing to the to address we are also pushing to the from address.

Scenario:

Alice has 3 positions.
position[0] = 100
position[1] = 200
position[2] = 300

Alice transfers position[0] to herself.

Looking at the code it will:

1. Push position[0] to the end of the array

position[0] = 100
position[1] = 200
position[2] = 300
position[3] = 100 <-- this is the position that was transferred

1. Delete position[0] from the array and set position[0] to position[1]'s value

position[0] = 200
position[1] = 200
position[2] = 300
position[3] = 100

1. repeat for position[1] by deleting position[1] and setting position[1] to position[2]'s value

position[0] = 200
position[1] = 300
position[2] = 300
position[3] = 100

At this point the looping is done since `feeDataArrayLength was only 3 (The value before "push" occurred).

1. Next it will delete the position at feeDataArrayLength - 1 which is position[2] (feeDataArrayLength - 1 = 3 - 1 = 2).

position[0] = 200
position[1] = 300
position[2] = 0
position[3] = 100

1. Lastly it will pop the last element from the array. Which is position[3] (The transferred position).

position[0] = 200
position[1] = 300
position[2] = 0

The consequence of this is that the user has lost their transferred position, as well as the ability to ever remove liquidity. since position[2] is 0. and when removing liquidity the code will divide by position[2] value which will cause a division by zero error.

Self-transfers are valid ERC721 operations and may be used by external protocols to:

• Trigger transfer events for dependent contracts

• Update state for the integrating protocol or other state logic

• Even without a specific reason to do so, self-transfers are valid ERC721 operations that is compliant with the standard. Transferring any other erc721 token would not cause any issues but transferring an LP NFT would cause the issue described above.

Impact

Loss of funds - Users who perform self-transfers will:

1. Lose their position data

2. Be unable to remove liquidity due to division by zero errors

3. Have their funds permanently locked in the protocol

Tools Used

Manual Review

Recommendations

Add a check for self-transfers and handle them appropriately: