Malicious primitive can distort Ocean's accounting of registered tokens and exploit other natively registered liquidity pools for profits

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L836 https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L1039-L1044

Vulnerability details

Impact

A malicious native primitive can exploit the Ocean.sol's accounting of registered tokens through users' computeOutputAmount transactions, and exploit other natively registered liquidity pools for profits.

Proof of Concept

In Ocean.sol's accounting process, Ocean's native primitives and ocean adapters are handled differently.

For ocean adapters that wrap external liquidity pools, unwrapToken()(OceanAdapter.sol) and wrapToken()(OceanAdapter.sol) are needed during _computeOutputAmount or _computeInputAmount interactions. And Ocean.sol will also force to mint wrapped inputTokens and burn wrapped outputTokens from the adapters, which is done through _increaseBalanceOfPrimitive and _decreaseBalanceOfPrimitive. For Ocean's natively registered primitives, Ocean.sol will not mint or burn if the inputToken or outputToken is registered by the same primitive concerned. This is achieved through if (_isNotTokenOfPrimitive(outputToken, primitive) && (outputAmount > 0)) bypass.

The double standard makes sense because based on code Doc in OceanERC1155.sol registerNewTokens(): @dev These are tokens that cannot be wrapped or unwrapped. The assumption is that when a primitive registers the outputToken, it doesn't need to wrap or unwrap beforehand or during computeOutputToken()/ computeInputToken(). The balance change is directly tracked in Ocean.sol. At the end of compute*(), Ocean will simply mint the outputToken for the user without burning the outputToken from the registered primitive.

There are (2) vulnerabilities here: (1) Ocean.sol only assumes that a native primitive that registered the outputToken doesn't need to wrap the registered token, but doesn't check that the native primitive will NOT wrap the registered outputToken; (2) Ocean.sol will in fact allow a native primitive to wrap the registered outputToken at any time through _erc20Wrap();

//src/ocean/Ocean.sol
    //@audit There are not check to prevent a registered token from being wrapped, or preventing primitive to wrap it's own registered Lptoken
    function _erc20Wrap(address tokenAddress, uint256 amount, address userAddress, uint256 outputToken) private {
        try IERC20Metadata(tokenAddress).decimals() returns (uint8 decimals) {
            /// @dev the amount passed as an argument to the external token
            uint256 transferAmount;
            /// @dev the leftover amount accumulated by the Ocean.
            uint256 dust;
            (transferAmount, dust) = _determineTransferAmount(amount, decimals);
            // If the user is unwrapping a delta, the residual dust could be
            // written to the user's ledger balance. However, it costs the
            // same amount of gas to place the dust on the owner's balance,
            // and accumulation of dust may eventually result in
            // transferrable units again.
            _grantFeeToOcean(outputToken, dust);
            SafeERC20.safeTransferFrom(IERC20(tokenAddress), userAddress, address(this), transferAmount);
            emit Erc20Wrap(tokenAddress, transferAmount, amount, dust, userAddress, outputToken);
        } catch {
            revert NO_DECIMAL_METHOD();
        }

(https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L820)

//src/ocean/Ocean.sol
    function _decreaseBalanceOfPrimitive(address primitive, uint256 outputToken, uint256 outputAmount) internal {
        // If the output token is not one of the primitive's tokens, the
        // primitive loses the output amount it just computed.
        // Otherwise, the tokens will be implicitly minted by the primitive
        // later in the transaction
        //@audit if the same primitive has registered the outputTOken, outputToken will not be burned from the primitive
        if (_isNotTokenOfPrimitive(outputToken, primitive) && (outputAmount > 0)) {
            _burn(primitive, outputToken, outputAmount);
        }

(https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/ocean/Ocean.sol#L1043-L1044)

These two open up attack vectors for a malicious Ocean native primitive to wrap its own registered outputTokens during compute*() calls.

Suppose this example:

The malicious primitive contract needs to a) inherit ERC20.sol, b) and inherit src/proteus/LiquidityPool.sol which will register its LpToken in Ocean. c) able to receive ERC1155 tokens, and transfer ERC1155 it received to an EOA.

Step 1: Malicious PrimitiveA register in Ocean by calling registerNewTokens(), which register is address as LpToken. Suppose the registered LpToken id is OceanIdA. PrimitiveA offers token swaps and liquidity management. xToken is WBTC, yToken is USDC.

Step 2: Before the attack, PrimitiveA will need to implement wrapping its LPToken OceanIdA in computeInputAmount() and computOutputAmount(). For example:

//src/proteus/LiquidityPool.sol
//Malicious implementation based on LiquidityPool.sol
    function computeOutputAmount(
        uint256 inputToken,
        uint256 outputToken,
        uint256 inputAmount,
        address userAddress,
        bytes32 minimumOutputAmount
    )
        external
        override
        onlyOcean
        returns (uint256 outputAmount)
    {
//Same logic for computing input/output amount here
//Add wrap LpToken here
      if(outputToken == lpTokenId) {
|>    wrapToken(outputToken, outputAmount);
      }
    }

Inside wrapToken(), PrimitiveA will mint its LPToken for outputAmount and approve Ocean.sol for outputAmount, before calling Ocean.sol to _erc20Wrap(). For example:

//Malicious wrapToken
   function wrapToken(uint256 tokenId, uint256 amount) internal override {
   //PrimitiveA inherits ERC20, and mint and approve to satisfy Ocean.sol `_erc20Wrap()` call
   _mint(address(this),amount);
   _approve(address(this), ocean, amount);
  //do interaction with Ocean.sol
          Interaction memory interaction = Interaction({
            interactionTypeAndAddress: _fetchInteractionId(address(this), uint256(InteractionType.WrapErc20)),
            inputToken: 0,
            outputToken: 0,
            specifiedAmount: amount,
            metadata: bytes32(0)
        });

        IOceanInteractions(ocean).doInteraction(interaction);

Now in compute*(), PrimitiveA will mint and transfer LpToken to Ocean.sol, in return Ocean.sol mint the wrapped OceanIdA for PrimitiveA's address balance.

For now on any user who deposits WBTC or USDC into primitiveA through Ocean.sol, the following will happen: a) the user will wrap WBTC and USDC into Ocean; b) Ocean will mint wrapped WBTC / USDC in the user address; c) In the same compute*(), PrimitiveA will mint and transfer LpToken to Ocean.sol; d) Ocean.sol increase PrimitveA's OceanIdA balance by amount; e) Ocean.sol will also increase the user's OceanIdA balance by amount;

(Note: Although LpToken in Step 2 is transferred to Ocean.sol, Ocean doesn't own these tokens and has no means to sweep tokens or regulate PrimitiveA's balance.)

Step 3: Overtime more users add liquidity to PrimtiveA. And PrimitiveA will accumulate its balance of wrapped OceanIdA in Ocean.sol. PrimitiveA's balance mirrored users combined OceanIdA balance.

Step 4: Case 1:Other ocean native primitives adopt OceanIdA as part of their accepted swap tokens. Suppose PrimitiveB supports Eth, Frax and OceanIdA as swap tokens and its LPToken id is OceanIdB. (See a similar scenario in the integration test: test/integration/Interaction.js, where poolB is deployed with poolA's lpToken as a swap token )

There will be users who add liquidity to PrimitiveB, depositing Eth, Frax or OceanIdA.

Now malicious primitive can mobilize its accumulated OceanIdA, transferring the wrapped OceanIdA to an EOA. The EOA will use its wrapped OceanIdA balance to swap out Eth or Frax. The EOA will unwrap the swapped-out Eth or Frax for profits.

Step 4: Case 2: The malicious PrimitiveA can also create and register another ocean-native liquidity pool that supports Eth, Frax and OceanIdA. The rest is the same as Case 1.

The attacker can potentially open multiple Ocean native liquidity pools that support a diversity of popular ERC20 tokens and perform the same swap for diversified profits.

It should be noted that it’s the ocean’s accounting that is compromised to enable the attack.

This is because registered LpTokens are not intended to be wrapped or unwrapped as stated in the code doc(registerNewTokens()). They only exist for balance accounting and are essentially ghost tokens. However, attacker can still mint an ERC20 lpToken and force wrap it with Ocean.sol, this contradicts the core accounting premise of Ocean. In this case, the total balance increase of OceanIdA doubles each time there is an add-liquidity by a user into the malicious primitive.

I consider this high due to the erosion of Ocean.sol's accounting premise.

Tools Used

Manual Review

Recommended Mitigation Steps

In Ocean.sol, check and revert when a native primitive tries to wrap its own LpToken. For example:

//src/ocean/Ocean.sol
    function _erc20Wrap(address tokenAddress, uint256 amount, address userAddress, uint256 outputToken) private {
    if(tokenAddress == userAddress) revert();
    //The rest of the logic here

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #54

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Out of scope

[flowercrimson]

Hey @0xA5DF, Thanks for judging. I don't think this is out of scope. The vulnerability lies in Ocean.sol, which is in scope. Ocean.sol interacts with all primitives and this report points to an edge case where a primitive that registered its LpTokens in Ocean can exploit Ocean.sol _erc20Wrap due to missing checks. And the fix is also in _erc20Wrap on Ocean.sol.

Based on doc, registered tokens @dev These are tokens that cannot be wrapped or unwrapped . However, _erc20Wrap on Ocean.sol doesn't check the token being wrapped is not a registered token. A primitive that mint its own registered token and wrap it through _erc20Wrap can exploit the accounting.

The attack path supposes a registered primitive to mint its own registered Lptoken and wrap it in Ocean.sol, every time a user deposit liquidity (swapping out registered Lptoken). A registered primitive minting its own registered Lptoken in Ocean.sol shouldn't be allowed, because it enables the primitive to accumulate 'ghost balance' of Lptoken in Ocean system. The 'ghost balance' can be repurposed to profit from other primitives in Ocean.

This is an exploit of _erc20Wrap 's vulnerability.

[0xA5DF]

See the comment in the primary issue, this is OOS because this is a known issue that was listed in the contest description (README.md)