Incorrect token contract address used when calculating WRAPPED_ETHER_ID inside the constructor will break the protocol

[c4-bot-5]

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/ocean/Ocean.sol#L173

Vulnerability details

WRAPPED_ETHER_ID is an immutable variable which is the ocean id for shETH. But while initializing it inside the constructor, the wrong hexadecimal value is used to calculate the ocean id.

Proof of Concept

Twice inside the Ocean contract it is mentioned that shETH is used for WRAPPED_ETHER_ID.

File: Ocean.sol

83:   /// @notice this is the oceanId used for shETH            /// @here
84:   /// @dev hexadecimal(ascii("shETH"))
85:   uint256 public immutable WRAPPED_ETHER_ID;

973:  * @dev Unwrap Ether out of the ocean.  The Ocean ID of shETH is computed    /// @here
974:  *  in the constructor using the address of the ocean and a tokenId of 0.

But inside the constructor, the hexadecimal value calculated corresponds to Ether & not shETH.

File: Ocean.sol

    constructor(string memory uri_) OceanERC1155(uri_) {
        ..CODE..
173:    WRAPPED_ETHER_ID = _calculateOceanId(address(0x4574686572), 0); // hexadecimal(ascii("Ether"))
    }

ASCII to Hexadecimal

• Ether => 0x4574686572
• shETH => 0x7368455448

WRAPPED_ETHER_ID value

• Ether => 68598205499637732940393479723998335974150219832588297998851264911405221787060
• shETH => 3016971713065753735091523569535506608175567490753644447457699582002654295551

Note:

CurveTricryptoAdapter contract specifically mentions - "curve tricrypto adapter contract enabling swapping, adding liquidity & removing liquidity for the curve usdt-wbtc-eth pool"

& the Ocean contract says "this is the oceanId used for shETH" for WRAPPED_ETHER_ID

Impact

The zToken inside contract CurveTricryptoAdapter.sol & WRAPPED_ETHER_ID inside contract Ocean.sol would be using the same Ocean Id for all their calculations leading to incorrect results & breaking the accounting system.

File: Ocean.sol

    constructor(string memory uri_) OceanERC1155(uri_) {
        ..CODE..
173:    WRAPPED_ETHER_ID = _calculateOceanId(address(0x4574686572), 0); // hexadecimal(ascii("Ether"))
    }

File: CurveTricryptoAdapter.sol

85:  constructor(address ocean_, address primitive_) OceanAdapter(ocean_, primitive_) {
        ..CODE..
100:    zToken = _calculateOceanId(address(0x4574686572), 0); // hexadecimal(ascii("Ether"))

For example, inside _etherUnwrap() in Ocean.sol, the fee would be sent to the wrong ocean id i.e. to the id of the zToken inside CurveTricryptoAdapter.sol.

File: Ocean.sol

988: function _etherUnwrap(uint256 amount, address userAddress) private {
999:   uint256 feeCharged = _calculateUnwrapFee(amount);
980:   _grantFeeToOcean(WRAPPED_ETHER_ID, feeCharged);
       ...

This will lead to incorrect balance of zToken whenever balanceOf() is called inside CurveTricryptoAdapter.sol.

Tools Used

Manual Review

Recommended Mitigation Steps

Use shETH instead of Ether for calculating ocean id.

File: Ocean.sol

- 173:    WRAPPED_ETHER_ID = _calculateOceanId(address(0x4574686572), 0); // hexadecimal(ascii("Ether"))
+ 173:    WRAPPED_ETHER_ID = _calculateOceanId(address(0x7368455448), 0); // hexadecimal(ascii("shETH"))

Assessed type

Error

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #46

[c4-pre-sort]

raymondfam marked the issue as not a duplicate

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[raymondfam]

Code and comment mismatch. Probably inconsequential.

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[0xA5DF]

Seems like a valid low, no significant impact was demonstrated

[c4-judge]

0xA5DF marked the issue as grade-c

[0xA5DF]

Low quantity, this is the only QA by the warden

[Shubh0412]

The above submission does point out the issue although the impact described in relation with CurveTricryptoAdapter.sol is incorrect.

However, the incorrect calculated id still posses an issue.

The above described issue shows an invariant being partially broken.

• Fees should be credited to the Ocean owner's ERC-1155 balance

Although the fees is being to the owner, it is sent to the incorrect Ocean ID because of wrong address used inside the constructor while calculating the WRAPPED_ETHER_ID.

The correct WRAPPED_ETHER_ID should have been calculated as follows

File: Ocean.sol
173:    WRAPPED_ETHER_ID = _calculateOceanId(address(0x7368455448), 0); // hexadecimal(ascii("shETH"))    /// @correct

& not as

File: Ocean.sol
173:    WRAPPED_ETHER_ID = _calculateOceanId(address(0x4574686572), 0); // hexadecimal(ascii("Ether"))    /// @incorrect

Because an invariant is being broken, the submission should be a Medium.

[0xA5DF]

Because an invariant is being broken, the submission should be a Medium.

I don't think that breaking an invariant alone changes the impact, the impact is judged based on what are the consequences of the issue. And even with the additional context that you've added I still think the appropriate severity is low.

[Shubh0412]

I accept that the original submission has incorrect impact & cannot quality for a H/M. However, the mentioned issue still persists & might not have been mentioned in any other submissions. Even though I haven't submitted any QA report, the said finding should still be considered as a valid Low instead of the being discarded as 'unsatisfactory' because of low quantity.

[0xA5DF]

It does qualify as a valid QA, see my comment in the PJQA thread regarding my method of grading QAs