code-423n4 / 2024-08-superposition-validation

0 stars 0 forks source link

tokenURI does not contain information about position which should not be the case #204

Closed c4-bot-4 closed 1 month ago

c4-bot-4 commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/sol/OwnershipNFTs.sol#L183

Vulnerability details

Impact

The protocol follows UniswapV3 protocol and implements NFT position manager. The problem is that the tokenURI does not contain any information about the position meaning that the user will only have tokenId but the position information will not be stored in the tokenId but somewhere else.

Proof of Concept

Currently tokenURI() function returns TOKEN_URI constant that's set in the constructor of the contract meaning only the owner will set the initial token URI. After that moment, all tokenIds will have the similar tokenURI which should not be the case for this protocol as it aims to have positions represented by ERC721 standard. But there is no any info currently about the position of the tokenId when creating a new tokenId:

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/sol/OwnershipNFTs.sol#L182-184

function tokenURI(uint256 /* _tokenId */) external view returns (string memory) {
        return TOKEN_URI;
    }

The only place that stores info about the position is position.rs contract that is not somehow connected to the tokenId from the OwnershipNFTs contract:

https://github.com/code-423n4/2024-08-superposition/blob/main/pkg/seawater/src/position.rs#L13-21

pub struct StoragePositionInfo {
    pub lower: StorageI32,
    pub upper: StorageI32,
    pub liquidity: StorageU128,
    pub fee_growth_inside_0: StorageU256,
    pub fee_growth_inside_1: StorageU256,
    pub token_owed_0: StorageU128,
    pub token_owed_1: StorageU128,
}

Therefore, the tokenID currently does not contain any information about underlying position which is a deviation from the intended behavior.

Tools Used

Manual review.

Recommended Mitigation Steps

Consider implementing additional functionality that would describe position info in the tokenURI of the tokenId. This can be used as an example implementation:

https://github.com/Uniswap/v3-periphery/blob/main/contracts/NonfungiblePositionManager.sol

Assessed type

Other

rodiontr commented 4 weeks ago

I believe this issue should be transferred to the findings repo for the following reasoning:

  1. The issue demonstrates how tokenURI functionality is misused for this particular type of the protocol with example of the correct implementation from UniswapV3

  2. This makes protocol not usable from the UI perspective as the NFT of a user has to contain information about his position.