code-423n4 / 2024-06-panoptic-findings

1 stars 0 forks source link

Inaccurate Premium Accounting in SFPM Due to Incomplete Data Updates in registerTokenTransfer. #40

Closed howlbot-integration[bot] closed 5 months ago

howlbot-integration[bot] commented 5 months ago

Lines of code

https://github.com/code-423n4/2024-06-panoptic/blob/153f0d82440b7e63075d55b0659706531431145f/contracts/SemiFungiblePositionManager.sol#L642-L647

Vulnerability details

Impact

registerTokenTransfer does not update the s_accountPremiumOwed and s_accountPremiumGross mappings when transferring tokens.

When a token is transferred from one address to another, the function correctly updates the s_accountLiquidity and s_accountFeesBase mappings to reflect the transfer of liquidity and fees. However, it does not update the s_accountPremiumOwed and s_accountPremiumGross mappings, which store the premium owed and gross premium for each account.

This means that after a token transfer, the premium-related data will still be associated with the original owner's address instead of being transferred to the new owner. This can lead to incorrect accounting of premiums and potentially affect the calculation of fees and other related functionalities.

The lack of updating the s_accountPremiumOwed and s_accountPremiumGross mappings during token transfers in the registerTokenTransfer function can lead to several issues:

  1. After a token transfer, the premium-related data will still be associated with the original owner's address instead of being transferred to the new owner. This means that the new owner's premium owed and gross premium will not be accurately reflected in the contract's state.

  2. The premium data is used in various fee calculations throughout the contract. If the premium data is not properly transferred to the new owner, it can lead to incorrect fee calculations and distributions.

    1. The registerTokenTransfer function updates the s_accountLiquidity and s_accountFeesBase mappings during token transfers: SemiFungiblePositionManager Contract (@contracts/SemiFungiblePositionManager.sol:642-647)
      
      // update+store liquidity and fee values between accounts
      s_accountLiquidity[positionKey_to] = fromLiq;
      s_accountLiquidity[positionKey_from] = LeftRightUnsigned.wrap(0);

s_accountFeesBase[positionKey_to] = fromBase; s_accountFeesBase[positionKey_from] = LeftRightSigned.wrap(0);

However, it does not update the `s_accountPremiumOwed` and `s_accountPremiumGross` mappings.

 2. The `s_accountPremiumOwed` and `s_accountPremiumGross` mappings are defined in the contract.
```solidity
mapping(bytes32 => LeftRightSigned) internal s_accountPremiumOwed;
mapping(bytes32 => LeftRightUnsigned) internal s_accountPremiumGross;
  1. The premium data is used in various functions throughout the contract, such as _getPremiaDeltas and _updateStoredPremia:
    
    function _getPremiaDeltas(
    bytes32 positionKey,
    uint256 premium0X64,
    uint256 premium1X64
    ) internal view returns (uint256 premium0X64_delta, uint256 premium1X64_delta) {
    LeftRightUnsigned memory premiumGross = s_accountPremiumGross[positionKey];
    // ...
    }

function _updateStoredPremia( bytes32 positionKey, uint256 premium0X64_delta, uint256 premium1X64_delta ) internal { s_accountPremiumOwed[positionKey] = s_accountPremiumOwed[positionKey].addLeftUnsigned( premium0X64_delta ).addRightUnsigned(premium1X64_delta); // ... }


These functions rely on the accurate premium data stored in the `s_accountPremiumOwed` and `s_accountPremiumGross` mappings.

## Proof of Concept
Consider the following scenario:

   1. Alice owns a token with ID 1 and has associated premium data stored in the contract.
   2. Alice transfers the token to Bob using the `safeTransferFrom` function, which internally calls `registerTokenTransfer`.
   3. After the transfer, the token ownership is updated correctly, and Bob becomes the new owner of the token.
   4. However, the premium data associated with the token remains linked to Alice's address in the `s_accountPremiumOwed` and `s_accountPremiumGross` mappings.
   5. If Bob tries to claim the premiums or if the contract performs fee calculations based on the premium data, it will use Alice's premium data instead of Bob's, leading to incorrect results.

This scenario demonstrates how the lack of updating the premium data during token transfers can lead to inconsistencies and potential issues in the contract's functionality.

## Tools Used
Vs Code

## Recommended Mitigation Steps
The function should also update the `s_accountPremiumOwed` and `s_accountPremiumGross` mappings during the token transfer process, similar to how it updates the liquidity and fees mappings. The premium data should be transferred from the sender's address to the recipient's address to ensure accurate tracking of premiums for each account.

## Assessed type

Error
Picodes commented 5 months ago

The PoC is incomplete as transfers are severely restricted and premium accumulators are not always relevants

c4-judge commented 5 months ago

Picodes marked the issue as unsatisfactory: Insufficient proof