oceanID is incorrectly calculated

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/adapters/OceanAdapter.sol#L108-L110 https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L709-L710

Vulnerability details

Impact

oceanID is not calculated the same way as it's defined in the Whitepaper.

Whitepaper defines two ways of calculating oceanID. For ERC-20 tokens, the Ocean ID is a cast of the ERC-20 contract’s address to a uint256. For ERC-721 and ERC-1155 tokens Ocean ID is derived from a hash of the contract’s address and the token ID. The current implementation does not follow this standard. ERC-20 tokens are calculated in the same way as ERC-721 and ERC-1155.

I wasn't quite sure how to evaluate this issue. It should be either evaluated as QA or Medium. I decided for Medium because of the following reasons. Usually, Code4rena evaluates errors in comments as QA. However, this case is different. This is not error in comment/docs - it's error in the codebase, which does not follow Whitepaper. Whitepaper straightforwardly defines how oceanIDs should be calculated. Protocols which integrate with Ocean and which follow its Whitepaper may not recognize tokens, if their oceanID won't be calculated properly. Since Ocean IDs are a critical part of the protocol - following defined in the Whitepaper standard is extremely important.

Proof of Concept

According to Ocean WhitePaper

3.1 Wrapped Tokens
Wrapped tokens are tokens held by the Ocean on an external ledger, with a proportional quantity
represented in the Ocean ledger. We use the prefix “sh-” to denote Shell-wrapped tokens. For
example, shUSDC is USDC wrapped into the Ocean. Ocean IDs for wrapped tokens are derived
from their attributes on the external ledger. For ERC-20 tokens, the Ocean ID is a cast of the
ERC-20 contract’s address to a uint256:

uint256 oceanID = uint256(uint160(contractAddress));

For ERC-721 and ERC-1155 tokens, there are potentially multiple tokens represented in each
smart contract. Therefore, we cannot use the contract’s address as the Ocean ID. We also cannot
use the token ID because that could result in a collision between two Ocean IDs. Instead, the
Ocean ID is derived from a hash of the contract’s address and the token ID:

uint256 oceanID = uint256(keccak256(abi.encodePacked(contractAddress, tokenID)));

ERC-20 Wrapped Tokens are calculated as:

uint256 oceanID = uint256(uint160(contractAddress));

While ERC-721 and ERC-1155 Wrapper Tokens are calculated as:

uint256 oceanID = uint256(keccak256(abi.encodePacked(contractAddress, tokenID)));

However, the adapter provides only one function to calculate Ocean ID, which is defined as below:

File: OceanAdapter.sol

    function _calculateOceanId(address tokenAddress, uint256 tokenId) internal pure returns (uint256) {
        return uint256(keccak256(abi.encodePacked(tokenAddress, tokenId)));
    }

This function is used in Ocean.sol, when we're wrapping or unwrapping ERC-20:

File: Ocean.sol

        if (interactionType == InteractionType.WrapErc20 || interactionType == InteractionType.UnwrapErc20) {
            specifiedToken = _calculateOceanId(externalContract, 0);

This implies, that whenever ERC-20 token is provided, it will be incorrectly calculated. For ERC-20 with address 0xAAA, _calculateOceanId() (according to the WhitePaper) should return:

uint256(uint160(0xAAA))

However, it will return:

uint256(keccak256(abi.encodePacked(0xAAA, 0)))

Tools Used

Manual code review

Recommended Mitigation Steps

Redesign _calculateOceanId() function, so that whenever it will receive ERC-20, it will return correct value, e.g.:

function _calculateOceanId(address tokenAddress, uint256 tokenId, bool isERC20) internal pure returns (uint256) {
        if (isERC20) return uint256(uint160(tokenAddress));
        return uint256(keccak256(abi.encodePacked(tokenAddress, tokenId)));
    }

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

uint256(uint160(0xAAA)) and uint256(keccak256(abi.encodePacked(0xAAA, 0))) probably return the same thing. The former automatically shifts the 20 bytes to the right whereas the latter packed 0xAAA and 0 without any padding.

[c4-sponsor]

viraj124 (sponsor) disputed

[viraj124]

the calculation is correct here, we call the method here https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/Curve2PoolAdapter.sol#L79 passing 0 so hence it is the same and in accordance with the whitepaper

[0xA5DF]

@viraj124, I think the warden is right about this one. The docs says that for ERC20 it'll just cast the address to uint256, without any hashing. In reality the code does hash (keccak256) ERC20s.

[viraj124]

yeah so we can mark this qa because it is a mistake in the doc and not the ocean itself the behaviour in the smart contract is intended @0xA5DF

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[0xA5DF]

I don't see any significant impact from this issue, therefore marking as low

[c4-judge]

0xA5DF marked the issue as grade-c

[c4-judge]

0xA5DF marked the issue as grade-a