Shell Protocol - oakcobalt's results

Low-01: Ocean.sol _doMultipleInteractions() doesn't check for duplicated ids input array (Note: not submitted in bot report)

In Ocean.sol _doMultipleInteractions() both the interactions array(Interaction[] calldata) and ocean Ids(uint256[] calldata) array required for the interactions are passed by callers. Note that interactions array and ids array can be of different lengths and the interactions array can contain duplicated interaction (e.g. adding the same liquidity twice, etc), however, Ids array shouldn't contain duplicated elements.

When Ids array contain the same OceanId twice, only the first index of the OceanId in the array will be correctly modified, the other indexes of duplicated OceanId will be left at default 0 value. The impact is every time all the indexes will be looped through in a for-loop _findIndexOfTokenId(), _copyDeltasToMintAndBurnArrays() from BalanceDelta.sol, this will increase unnecessary number of iterations, every time a duplicated id is passed to _doMultipleInteractions(). The more ocean ids involved in the transaction, the more wasteful iterations of the duplicated ids it will generate.

//src/ocean/Ocean.sol
    function _doMultipleInteractions(
        Interaction[] calldata interactions,
        uint256[] calldata ids,
        address userAddress
    )
...
        // Use the passed ids to create an array of balance deltas, used in
        // the intra-transaction accounting system.
        //@audit duplicated ids will create addition empty elements in balancesDeltas
|>      BalanceDelta[] memory balanceDeltas = new BalanceDelta[](ids.length);
        uint256 _idLength = ids.length;
        for (uint256 i = 0; i < _idLength;) {
            balanceDeltas[i] = BalanceDelta(ids[i], 0);
            unchecked {
                ++i;
            }
        }
...
        balanceDeltas.increaseBalanceDelta(WRAPPED_ETHER_ID, msg.value);

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

//src/ocean/BalanceDelta.sol
    function increaseBalanceDelta(
        BalanceDelta[] memory self,
        uint256 tokenId,
        uint256 amount
    )
...
|>      uint256 index = _findIndexOfTokenId(self, tokenId);
        self[index].delta += int256(amount);
        return;

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

//src/ocean/BalanceDelta.sol
    function _findIndexOfTokenId(BalanceDelta[] memory self, uint256 tokenId) private pure returns (uint256 index) {
|>      for (index = 0; index < self.length; ++index) {
            if (self[index].tokenId == tokenId) {
                return index;
            }
        }

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

//src/ocean/BalanceDelta.sol
    function _copyDeltasToMintAndBurnArrays(
        BalanceDelta[] memory self,
        uint256[] memory mintIds,
        uint256[] memory mintAmounts,
        uint256[] memory burnIds,
        uint256[] memory burnAmounts
    )
...
         //@audit This will iterate through all BalanceDelta elements even those with duplicated Ids
|>       for (uint256 i = 0; i < self.length; ++i) {
            int256 delta = self[i].delta;

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

Recommendations: Consider check for duplicates in the initial ids array in _doMultipleInteractions and remove duplicated id element before declaring BalanceDelta[]. This removes unnecessary for-loop iterations later on in the transaction.

Low-02: In Curve2PoolAdapter.sol constructor(), there is unnecessary external call to approve primitive_ to spend primitive_

In src/adapters/Curve2PoolAdapter.sol _approveToken(), for a given tokenAddress input, this will approve both ocean and primitive to spend token on behalf of the adaptor.

In the constructor, _approveToken() is called for xTokenAddress, yTokenAddress, and also primitive_. In the last case, _approveToken() will approve primitive_ to spend primitive_ on behalf of the adapter. This is an unnecessary external call, because primitive_ will be curve usdc-usdt pool address, and curve2Pool will not need to call itself with transferFrom function selector, since when removing liquidity, Curve usdc-usdt pool will directly reduce the Curve2PoolAdapter's balance.

//src/adapters/Curve2PoolAdapter.sol
    constructor(address ocean_, address primitive_) OceanAdapter(ocean_, primitive_) {
...
        lpTokenId = _calculateOceanId(primitive_, 0);
        underlying[lpTokenId] = primitive_;
        decimals[lpTokenId] = IERC20Metadata(primitive_).decimals();
        //@audit _approveToken will also approve `primitive_` to spend `primitive_` 
|>      _approveToken(primitive_);

(https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/adapters/Curve2PoolAdapter.sol#L94)

//src/adapters/Curve2PoolAdapter.sol
    function _approveToken(address tokenAddress) private {
        IERC20Metadata(tokenAddress).approve(ocean, type(uint256).max);
        //@audit when _approveToken(primitive_) is called in constructor, this make an unnecessary external call to curve usdc-usdt pool.
|>      IERC20Metadata(tokenAddress).approve(primitive, type(uint256).max);
    }

//Curve usdc-usdt pool compatible with Curve2PoolAdapter.sol
def remove_liquidity_one_coin(
    _burn_amount: uint256,
    i: int128,
    _min_received: uint256,
    _receiver: address = msg.sender,
) -> uint256:
...
|>  self.balanceOf[msg.sender] -= _burn_amount

(https://arbiscan.io/token/0x7f90122bf0700f9e7e1f688fe926940e8839f353#code) As seen above, when removing liquidity, Curve2PoolAdapter's primitive_ balance will be directly subtracted without the need for transferFrom primitive_. So no approving primitive_ for primitve_ is needed.

Recommendations: Because in this case primitive_ is the same as Lp token address, consider in Curve2PoolAdapter.sol _approveToken() add a bypass such that only when tokenAddress !=primitive,`IERC20Metadata(tokenAddress).approve(primitive, type(uint256).max) will be called.

Low-03: _convertDecimals() are declared in both OceanAdapter.sol and Ocean.sol with minor differences, consider refactoring this function as part of a library contract (Note: Not included in bot report)

In Ocean.sol and OceanAdapter.sol _convertDecimals() are declared twice with almost identical implementations except that in Ocean.sol, truncatedAmount is assigned and returned, while in OceanAdapter.sol truncatedAmount is not handled and returned.

//src/adapters/OceanAdapter.sol
    function _convertDecimals(
        uint8 decimalsFrom,
        uint8 decimalsTo,
        uint256 amountToConvert
    )
        internal
        pure
        returns (uint256 convertedAmount)
    {
        if (decimalsFrom == decimalsTo) {
            // no shift
            convertedAmount = amountToConvert;
        } else if (decimalsFrom < decimalsTo) {
            // Decimal shift left (add precision)
            uint256 shift = 10 ** (uint256(decimalsTo - decimalsFrom));
            convertedAmount = amountToConvert * shift;
        } else {
            // Decimal shift right (remove precision) -> truncation
            uint256 shift = 10 ** (uint256(decimalsFrom - decimalsTo));
            convertedAmount = amountToConvert / shift;
        }
    }

(https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/adapters/OceanAdapter.sol#L138-L159)

//src/ocean/Ocean.sol
    function _convertDecimals(
        uint8 decimalsFrom,
        uint8 decimalsTo,
        uint256 amountToConvert
    )
        internal
        pure
        returns (uint256 convertedAmount, uint256 truncatedAmount)
    {
        if (decimalsFrom == decimalsTo) {
            // no shift
            convertedAmount = amountToConvert;
            truncatedAmount = 0;
        } else if (decimalsFrom < decimalsTo) {
            // Decimal shift left (add precision)
            uint256 shift = 10 ** (uint256(decimalsTo - decimalsFrom));
            convertedAmount = amountToConvert * shift;
            truncatedAmount = 0;
        } else {
            // Decimal shift right (remove precision) -> truncation
            uint256 shift = 10 ** (uint256(decimalsFrom - decimalsTo));
            convertedAmount = amountToConvert / shift;
            truncatedAmount = amountToConvert % shift;
        }
    }

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

Recommendation: Consider moving decimals conversion into a library contract. In OceanAdapter.sol, the same _convertDecimals() from Ocean.sol can still be used.

NC-01 Some functions in abstract OceanAdapter can be refactored

OceanAdapter.sol is an abstract contract intended to be overwritten by child contract with specific implementations suited for corresponding external liquidity pools. For example, Curve2PoolAdapter.sol overwrites OceanAdapter.sol to wrap curve usdc-usdt pool. In this case, OceanAdapter should avoid specifying implementation that is not general and refactor these into the child contract.

One example is computeInputAmount() in OceanAdapter.sol. Current computeInputAmount() has revert() that could be implemented in Child contracts instead.

//src/adapters/OceanAdapter.sol
    function computeInputAmount(
        uint256 inputToken,
        uint256 outputToken,
        uint256 outputAmount,
        address userAddress,
        bytes32 maximumInputAmount
    )
       external
        override
        onlyOcean
        returns (uint256 inputAmount)
    {
        revert();
    }

(https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/adapters/OceanAdapter.sol#L93)

Recommendations: Consider moving revert() to be implemented in the child contract.

NC-02 Curve2PoolAdapter.sol doesn't work with all Curve2Pools currently implemented by Curve

Based on the code comment, the current implementation of Curve2PoolAdapter.sol is designed for curve usdc-usdt pool. However, Curve2PoolAdapter.sol will not work with current Curve2Pools that use a LpToken contract separate from the pool contract.

For example, current implementation will not work with Curve Crv-Eth pool. See here. Curve Crv-Eth pool uses a separate LpToken contract -Curve Crv-Eth token.

//Curve Crv-Eth pool
# These addresses are replaced by the deployer
//@audit in Crv-Eth pool, a separate hardcoded LpToken address is defined, different from pool address itself.
token: constant(address) = 0xEd4064f376cB8d68F770FB1Ff088a3d0F3FF5c4d
coins: constant(address[N_COINS]) = [
    0xC02aaA39b223FE8D0A0e5C4F27eAD9083C756Cc2,
    0xD533a949740bb3306d119CC777fa900bA034cd52]

(https://etherscan.io/address/0x8301ae4fc9c624d1d396cbdaa1ed877821d7c511#code)

//Curve Crv-Eth LpToken
"""
@title Curve LP Token
@author Curve.Fi
@notice Base implementation for an LP token provided for
        supplying liquidity to `StableSwap`
@dev Follows the ERC-20 token standard as defined at
     https://eips.ethereum.org/EIPS/eip-20
"""

(https://etherscan.io/address/0xEd4064f376cB8d68F770FB1Ff088a3d0F3FF5c4d#code)

In this case, Curve2PoolAdapter.sol will not work because it only defines xToken, yToken and primitive as input or output token, when it should be xToken, yToken and LpToken as input or output token.

//src/adapters/Curve2PoolAdapter.sol
    constructor(address ocean_, address primitive_) OceanAdapter(ocean_, primitive_) {
...
      address xTokenAddress = ICurve2Pool(primitive).coins(0);
        xToken = _calculateOceanId(xTokenAddress, 0);
        underlying[xToken] = xTokenAddress;
        decimals[xToken] = IERC20Metadata(xTokenAddress).decimals();
        _approveToken(xTokenAddress);

...
        address yTokenAddress = ICurve2Pool(primitive).coins(1);
        yToken = _calculateOceanId(yTokenAddress, 0);
        indexOf[yToken] = int128(1);
        underlying[yToken] = yTokenAddress;
        decimals[yToken] = IERC20Metadata(yTokenAddress).decimals();
        _approveToken(yTokenAddress);
...
        //@audit this is incorrect when curve2pool use a separate lpToken contract different from primitive contract such as crv-eth pool.
|>      lpTokenId = _calculateOceanId(primitive_, 0);
        underlying[lpTokenId] = primitive_;
        //@audit this will also revert due to primitive_ has no decimal methods
|>      decimals[lpTokenId] = IERC20Metadata(primitive_).decimals();
        _approveToken(primitive_);

(https://github.com/code-423n4/2023-11-shellprotocol/blob/485de7383cdf88284ee6bcf2926fb7c19e9fb257/src/adapters/Curve2PoolAdapter.sol#L91-L94)

In addition, development will fail due to primitive contract has no decimal method since it's not intended to be ERC20.

Recommendations: (1) Either develop a different version of Curve2PoolAdapter that work with Curve pool that has a separate LpToken; (2) Or revise current Curve2PoolAdapter to always use xToken, yToken and LpToken. And in the constructor, bypass when calling .token() method on curve pool fails which indicate it doesn't use a separate LPToken.

NC-03 OceanAdapter.sol computOutputAmount() will not handle the case for erc20->erc721.

In Ocean, erc20, erc721 and erc1155 are all supported. Theoretically, inputToken or outputToken can be erc721, erc20 or erc1155, depending the specific external liquidity protocols that OceanAdapter is wrapping. But this is currently not possible, which reduces Ocean's compatibility.

Although OceanAdapter.sol is an abstract contract, it has complete implementation for computOutputAmount(). It should be noted that current computOutputAmount() might not work when the outputToken needs to be ERC721. For example, depositing liquidity and the external pool mints ERC721 as an lp token.

This is because when wrap the output token after external add-liquidity call, OceanAdapter will directly use the outputToken as input for wrapToken(). And outputToken will be different from user input, because the minted ERC721 tokenId is determined on the fly by external pool.

In addition, primitiveOutputAmount() only returns one parameter outputAmount. In the case when the output Token is ERC721, tokenId will be needed to be wrapped in Ocean.sol.

These makes the erc20->erc721 use case errorneous, unless overwriting the logic of OceanAdapter computOutputAmount(). In addition, computOutputAmount() will also need to return both outputToken and outputAmount to be processed in Ocean.sol

I think this is NC severity due to OceanAdpater can be essentially overwritten by the child contract with new logic in computOutputAmount() if there is a need later on. Still, it would be better to account for this use case and refactor the code for increased compatibility, readability and avoid extensive overwriting.

Recommendations: (1) Consider having primitiveOutputAmount() return two variables (outputToken, outputAmount). And change computeOutputAmount() to pass the two values to wrapToken(). (2) Then the logic of handling (a) whether the outputToken requested by user is a ERC721, for example, user put _calcualteOceanId(externalERC721address, 0) as a placeholder for ERC721 (b) return outputToken, can be done in primitiveOutputAmount(). (3) allow OceanAdapter.sol computeOutputAmount()to return two variables (outputToken, outputAmount) (4) In Ocean.sol, _computeOutputAmount(), return both (outputToken, outputAmount) and pass them to _decreaseBalanceOfPrimitive(). (5) In Ocean.sol, _computeOutputAmount() will also return (outputToken, outputAmount), which will be passed to return values in _executeInteraction().

Summary

Description:

Shell Protocol is a collection of Ethereum Virtual Machine (EVM)-based smart contracts deployed on the Arbitrum One blockchain. Unlike traditional DeFi protocols that use single-purpose smart contracts, Shell stands out as a hub for a modular ecosystem of services. Its unique design facilitates the bundling of multiple smart contracts or the creation of new ones, providing users with a streamlined process to combine various services within a single transaction.

Existing Standards:

• The protocol adheres to conventional Solidity and Ethereum practices, primarily utilizing the ERC20, ERC721 and ERC1155 standards.
• The protocol also uses ERC165 standard for Ocean.sol for introspection.

1- Approach:

• Scope: The scope is limited to Ocean.sol and the adapter implementations. But extra attention is given also to LiquidityPool.sol, and BalanceDelta.sol for a more inclusive review of various use cases related to Ocean.sol and adapters.
• Roles: Main focus of user flows concerning the Ocean owner, general user that add liquidity, remove liquidity, swap or wrap tokens, or primitive/adapter creators.

2- Centralization Risks:

Overall low centralization risks given the limited number of access-controlled or admin-controlled functions.

Ocean.sol

• Owner is able to set fee division factor, which is a Dao-controlled process. This is low risk in centralization.
• Note that the owner flows are currently inadequately supported in Ocean.sol. Key owner flow such as fee claim and redistribution needs to go through general user methods such as _erc20unwrap(). This is good for decentralization but impedes the efficiency of owner fee management, and also will trap part of owner entitled fees in the contract. See my H/M submission for more information.

3- Systemic Risks:

Overall low systemic risks given the token exchange in Ocean.sol will essentially take place between primitive contracts and user accounts. Ocean.sol isn’t directly exposed to loss or gain from the user or primitive.

Slippage is also taken care of in Ocean liquidity pool or Ocean adapter contracts.

Counterparty Risks:

There are increased counterparty risks from malicious primitives (external liquidity pools, adapter contracts, or Ocean native liquidity pools).

Although the impact is contained between primitive and users, Ocean has a small exposure to such risks in the fee management and redistribution process.

Whenever a malicious token is wrapped in Ocean.sol, part of the fee is collected in the malicious token format and wrapped in Ocean id for the owner. Depending on the exact process of the ocean owner’s fee redistribution, a malicious token might DOS the entire fee unwrap and redistribution process.

4- Mechanism Review:

Inconsistent interaction orders:

Ocean.sol ‘s doMultipleInteractions() might require different interaction orders even for the same token pairs. This can be quite confusing to the general users to know which order is the correct order of interactions to pass to doMultipleInteractions(). This compromises the user experience.

The proper interaction orders depend on the implementation of the primitive contract and might not be known or fully understood by the user ahead of time.

Suppose this example:

A user wants to add liquidity by depositing TokenA through computeOutputAmount method. The user doesn’t have any token wrapped in Ocean. The user invokes doMultiplerInteractions().

(1) Case 1: The primitive is native ocean LiquidityPool that registered LP tokens.

Valid interaction order A: computeOutputAmount() → wrapErc20(). Since all token mints and burnt are taken care of at the end of doMultiplerInteractions(). Ocean.sol will directly mint input tokens (TokenA) for the primitive ahead of time, and user’s input token can be wrapped afterwards.

Valid interaction order B: wrapErc20() → computeOutputAmount(). The reverse order will also work for the same reasons above.

(2) Case 2: The primitive is an ocean adapter wrapping an external liquidity pool.

Valid interaction order B: wrapErc20() → computeOutputAmount(). This is the only valid interaction order in this case, because if Ocean.sol doesn’t have any wrapped TokenA ahead of time, the ocean adapter will not receive and unwrap any TokenA input token to be sent to the external liquidity pool. In this case the reverse order will cause revert.

No On-Chain Mechanism for Fee Redistribution:

In Ocean.sol, fees are minted for various wrapped tokens to owner(). However, there is no on-chain method to list all token ids that have fees collected for the owner for easy batch distribution. Currently, this information can only be tracked through event listening of minting. Consider on-chain implementation for queries on all tokens owned by Ocean owner.

computeInputAmount doesn't need to be disabled in OceanAdapter.sol

In OceanAdapter.sol, the abstract generalized adapter, computeInputAmount is implemented with a direct revert(). Although this revert() can be overwritten in deriving contracts, this is still unnecessary and potentially misleading.

(1) OceanAdapter.sol is an abstract contract and can be overwritten. Consider refactoring revert() implementation in Curve2PoolAdapter and CurveTricryptoAdapter. The abstract contract is better remained as a neutral contract unbiased of any specific swapping or liquidity management methods which might differ based on external protocols.

(2) computeIntputAmount can still be supported for ocean adapters if the external liquidity provides a compatible method. However, the issue is Ocean.sol doesn't implement _computeInputAmount interaction correctly. In Ocean.sol _computeInputAmount(), the order in which the primitive's balance of output token is decreased before a call to the primitive's computInputAmount() is problematic for any ocean adapters. Ocean.sol computInputAmount()'s implementation needs to be revised to allow ocean adapters' computeInputAmount to properly work. See my H/M submission for details.

Curve2PoolAdapter will not work with all Curve 2Pools:

Some curve 2 pools have a separate LpToken contract which is different from the liquidity pool contract. These pools will not work with Curve2PoolAdapter. See my QA submission for details.

For example, Curve 2 pools on Arbitrum are generally compatible with Curve2PoolAdapter. See below. (https://www.geckoterminal.com/arbitrum/curve_arbitrum/pools)

However, Curve 2 pools on Mainnet are generally incompatible. See below. (https://www.geckoterminal.com/eth/curve/pools)

It should also be noted that, generally curve 2 pools on arbitrum are compatible but it doesn’t prevent any future curve 2 pool deployment or other derivative implementation on arbitrum to become incompatible.

OceanAdapter currently doesn’t support some mainstream swapping and liquidity management protocols:

OceanAdapter.sol is intended to be generalized abstract contract that allows protocol and pool-specific implementation to inherit and overwrite. However, OceanAdapter currently is not compatible with a few common swapping or liquidity management methods such as two-sided liquidity adding, two-sided liquidity removing, ERC721 liquidity position minting, etc. Any external liquidity pools that adopt the above-mentioned methods will be incompatible. This include Uniswap V2 / V3. (1) Two-sided liquidity adding:

This requires two input tokens and one output token in one function call. Currently not supported.

(2) Two-sided liquidity removing:

This requires one input token and two output tokens in one function call. Currently not supported.

(3) ERC721 liquidity position minting:

This requires OceanAdapter to support receiving ERC721 tokens. Currently, OceanAdapter only supports receiving ERC1155.

(4) ERC721 liquidity position removing:

See reason for (3).

Time spent:

30 hours