Shell Protocol - 0xmystery's results

[L-01] Ocean._calculateUnwrapFee() should be rounded up

feeCharged should adopt the ceil instead of floor behavior of integer division in favor of updating the DAO's balance.

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

    function _calculateUnwrapFee(uint256 unwrapAmount) private view returns (uint256 feeCharged) {
        feeCharged = unwrapAmount / unwrapFeeDivisor;
    }  

I recommend adopting a math library to better handle the rounding matter.

[L-02] Unnecessary zero dust placing on the owner's balance

In Ocean._erc20Wrap(), dust returned by _determineTransferAmount() could be 0. In fact, dust typically/mostly equals 0, e.g. a 168 USDC amount of the ERC-20 token to be wrapped in terms of 18-decimal fixed point would be like 168_000000_000000000000 where dust == 0.

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

        } else {
            // if truncated == 0, then we don't need to do anything fancy to
            // determine transferAmount, the result _convertDecimals() returns
            // is correct.
            dust = 0;
        }

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

            (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);

I recommend having the affected code line refactored as follows:

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

-            _grantFeeToOcean(outputToken, dust);
+            if (dust != 0) { 
+                _grantFeeToOcean(outputToken, dust);
+            }

[L-03] Dust associated with token decimals over 18 not catered for

When dealing with an ERC-20 token whose decimals() is greater than 18, the portion beyond the 18th decimal place is discarded outright. For instance, when rawOutputAmount returned by Curve2PoolAdapter.primitiveOutputAmount() for a token with 21 decimals was 123.123456789012345678901, _convertDecimals() would be invoked to assign outputAmount as 123.123456789012345678 to be wrapped via wrapToken(), leaving/truncating 0.000000000000000000901.

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/Curve2PoolAdapter.sol#L162-L173

        if (action == ComputeType.Swap) {
            rawOutputAmount =
                ICurve2Pool(primitive).exchange(indexOfInputAmount, indexOfOutputAmount, rawInputAmount, 0);
        } else if (action == ComputeType.Deposit) {
            uint256[2] memory inputAmounts;
            inputAmounts[uint256(int256(indexOfInputAmount))] = rawInputAmount;
            rawOutputAmount = ICurve2Pool(primitive).add_liquidity(inputAmounts, 0);
        } else {
            rawOutputAmount = ICurve2Pool(primitive).remove_liquidity_one_coin(rawInputAmount, indexOfOutputAmount, 0);
        }

        outputAmount = _convertDecimals(decimals[outputToken], NORMALIZED_DECIMALS, rawOutputAmount);

I recommend adding a function that could be used to withdraw the accumulated truncated amount by the owner when it has become sizable enough to be transferable rather than leaving it stuck forever in the contract.

[L-04] Slippage could have been detected earlier

Curve2PoolAdapter.primitiveOutputAmount() and CurveTricryptoAdapter.primitiveOutputAmount() both have 0 slippage adopted by default when doing deposit, swap or withdraw, and doing the slippage check only after the rawOutputAmount has been converted to the 18 decimal format.

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/Curve2PoolAdapter.sol#L162-L175

        if (action == ComputeType.Swap) {
            rawOutputAmount =
                ICurve2Pool(primitive).exchange(indexOfInputAmount, indexOfOutputAmount, rawInputAmount, 0);
        } else if (action == ComputeType.Deposit) {
            uint256[2] memory inputAmounts;
            inputAmounts[uint256(int256(indexOfInputAmount))] = rawInputAmount;
            rawOutputAmount = ICurve2Pool(primitive).add_liquidity(inputAmounts, 0);
        } else {
            rawOutputAmount = ICurve2Pool(primitive).remove_liquidity_one_coin(rawInputAmount, indexOfOutputAmount, 0);
        }

        outputAmount = _convertDecimals(decimals[outputToken], NORMALIZED_DECIMALS, rawOutputAmount);

        if (uint256(minimumOutputAmount) > outputAmount) revert SLIPPAGE_LIMIT_EXCEEDED();

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/CurveTricryptoAdapter.sol#L198-L227

        if (action == ComputeType.Swap) {
            bool useEth = inputToken == zToken || outputToken == zToken;

            ICurveTricrypto(primitive).exchange{ value: inputToken == zToken ? rawInputAmount : 0 }(
                indexOfInputAmount, indexOfOutputAmount, rawInputAmount, 0, useEth
            );
        } else if (action == ComputeType.Deposit) {
            uint256[3] memory inputAmounts;
            inputAmounts[indexOfInputAmount] = rawInputAmount;

            if (inputToken == zToken) IWETH(underlying[zToken]).deposit{ value: rawInputAmount }();

            ICurveTricrypto(primitive).add_liquidity(inputAmounts, 0);
        } else {
            if (outputToken == zToken) {
                uint256 wethBalance = IERC20Metadata(underlying[zToken]).balanceOf(address(this));
                ICurveTricrypto(primitive).remove_liquidity_one_coin(rawInputAmount, indexOfOutputAmount, 0);
                IWETH(underlying[zToken]).withdraw(
                    IERC20Metadata(underlying[zToken]).balanceOf(address(this)) - wethBalance
                );
            } else {
                ICurveTricrypto(primitive).remove_liquidity_one_coin(rawInputAmount, indexOfOutputAmount, 0);
            }
        }

        uint256 rawOutputAmount = _getBalance(underlying[outputToken]) - _balanceBefore;

        outputAmount = _convertDecimals(decimals[outputToken], NORMALIZED_DECIMALS, rawOutputAmount);

        if (uint256(minimumOutputAmount) > outputAmount) revert SLIPPAGE_LIMIT_EXCEEDED();

I recommend converting minimumOutputAmount to the supposed token decimal format (just like it has been done so on inputAmount) via _convertDecimals() (round it up if need be), and then using the converted slippage amount instead of 0 when calling exchange(), add_liquidity() or remove_liquidity_one_coin().

[L-05] USDT might turn into a fee-on-transfer token

According to the contract NatSpec of Curve2PoolAdapter.sol, the adapter is enabling deposit, swap, and withdraw on the USDC-USDT pair on curve:

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/Curve2PoolAdapter.sol#L16-L19

/**
 * @notice
 *   curve2pool adapter contract enabling swapping, adding liquidity & removing liquidity for the curve usdc-usdt pool
 */

Nevertheless, USDT can activate fees and become a fee-on-transfer token, breaking all associated primitive interactions.

I recommend planning for this contingency by having a graceful primitive exit/cease route from the Ocean.

[L-06] Function arguments not efficiently used

When deploying Curve2PoolAdapter.sol and CurveTricryptoAdapter.sol, the inputted parameter primitive_ could have been fully utilized instead of resorting to the immutable primitive.

I recommend refactoring the affected codes as follows:

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/Curve2PoolAdapter.sol#L77-L95

    constructor(address ocean_, address primitive_) OceanAdapter(ocean_, primitive_) {
-        address xTokenAddress = ICurve2Pool(primitive).coins(0);
+        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);
+        address yTokenAddress = ICurve2Pool(primitive_).coins(1);
        yToken = _calculateOceanId(yTokenAddress, 0);
        indexOf[yToken] = int128(1);
        underlying[yToken] = yTokenAddress;
        decimals[yToken] = IERC20Metadata(yTokenAddress).decimals();
        _approveToken(yTokenAddress);

        lpTokenId = _calculateOceanId(primitive_, 0);
        underlying[lpTokenId] = primitive_;
        decimals[lpTokenId] = IERC20Metadata(primitive_).decimals();
        _approveToken(primitive_);
    }

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/CurveTricryptoAdapter.sol#L85-L111

    constructor(address ocean_, address primitive_) OceanAdapter(ocean_, primitive_) {
-        address xTokenAddress = ICurveTricrypto(primitive).coins(0);
+        address xTokenAddress = ICurveTricrypto(primitive_).coins(0);
        xToken = _calculateOceanId(xTokenAddress, 0);
        underlying[xToken] = xTokenAddress;
        decimals[xToken] = IERC20Metadata(xTokenAddress).decimals();
        _approveToken(xTokenAddress);

-        address yTokenAddress = ICurveTricrypto(primitive).coins(1);
-        address yTokenAddress = ICurveTricrypto(primitive_).coins(1);
        yToken = _calculateOceanId(yTokenAddress, 0);
        indexOf[yToken] = 1;
        underlying[yToken] = yTokenAddress;
        decimals[yToken] = IERC20Metadata(yTokenAddress).decimals();
        _approveToken(yTokenAddress);

-        address wethAddress = ICurveTricrypto(primitive).coins(2);
+        address wethAddress = ICurveTricrypto(primitive_).coins(2);
        zToken = _calculateOceanId(address(0x4574686572), 0); // hexadecimal(ascii("Ether"))
        indexOf[zToken] = 2;
        underlying[zToken] = wethAddress;
        decimals[zToken] = NORMALIZED_DECIMALS;
        _approveToken(wethAddress);

-        address lpTokenAddress = ICurveTricrypto(primitive).token();
+        address lpTokenAddress = ICurveTricrypto(primitive_).token();
        lpTokenId = _calculateOceanId(lpTokenAddress, 0);
        underlying[lpTokenId] = lpTokenAddress;
        decimals[lpTokenId] = IERC20Metadata(lpTokenAddress).decimals();
        _approveToken(lpTokenAddress);
    }

[L-07] Ineffective CEI adapted from OpenZeppelin Reentrancy Guard

In Ocean.sol, sandwiching the function logic of _erc721Wrap() and _erc1155Wrap() with NOT_INTERACTION and INTERACTION is essentially not doing much as far as reentrancy guard is concerned in the absence of any conditional or modifier check(s):

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

    function _erc721Wrap(address tokenAddress, uint256 tokenId, address userAddress, uint256 oceanId) private {
        _ERC721InteractionStatus = INTERACTION;
        IERC721(tokenAddress).safeTransferFrom(userAddress, address(this), tokenId);
        _ERC721InteractionStatus = NOT_INTERACTION;
        emit Erc721Wrap(tokenAddress, tokenId, userAddress, oceanId);
    }

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

    function _erc1155Wrap(
        address tokenAddress,
        uint256 tokenId,
        uint256 amount,
        address userAddress,
        uint256 oceanId
    )
        private
    {
        if (tokenAddress == address(this)) revert NO_RECURSIVE_WRAPS();
        _ERC1155InteractionStatus = INTERACTION;
        IERC1155(tokenAddress).safeTransferFrom(userAddress, address(this), tokenId, amount, "");
        _ERC1155InteractionStatus = NOT_INTERACTION;
        emit Erc1155Wrap(tokenAddress, tokenId, amount, userAddress, oceanId);
    }  

[L-08] Private function with embedded modifier reduces contract size

Consider having the logic of a modifier embedded through a private function to reduce contract size if need be. A private visibility that saves more gas on function calls than the internal visibility is adopted because the modifier will only be making this call inside the contract.

For instance, the modifier inside the big Ocean contract may be refactored as follows:

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

+    function _onlyApprovedForwarder(address userAddress) private view {
+        if (!isApprovedForAll(userAddress, msg.sender)) revert FORWARDER_NOT_APPROVED();
+    }

      modifier onlyApprovedForwarder(address userAddress) {
-        if (!isApprovedForAll(userAddress, msg.sender)) revert FORWARDER_NOT_APPROVED();
+        _onlyApprovedForwarder(address userAddress);
      _;
      }

[L-09] Optional deadline parameter for primitiveOutputAmount()

This is unlikely to happen on Arbitrum or any other L2 chain, but, should the protocol plan on making the Ocean-Primitive interactions on the Ethereum mainnet, a deadline option could further protect callers from changes beyond the mitigation capability offered by the slippage. For instance, swapping USDC for USDT a day later would be getting a higher rawOutputAmount.

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/Curve2PoolAdapter.sol#L162-L164

        if (action == ComputeType.Swap) {
            rawOutputAmount =
                ICurve2Pool(primitive).exchange(indexOfInputAmount, indexOfOutputAmount, rawInputAmount, 0);

However, due to congestion and bot MEV re-ordering, a deFi call transacted a day later would not have an expectedly higher rawOutputAmount received simply because a higher minimumOutputAmount had not been entered for better slippage protection.

[L-10] Excess contract balance not catered for

Ideally, a well-designed smart contract should not retain excess funds inadvertently. Any excess balance remaining after operations should be accounted for and handled appropriately, either by returning to the user, contributing to the next operation, or managed through specific functions designed for balance withdrawal or reallocation.

For instance, in CurveTricryptoAdapter.sol, ETH could be sent in accidentally via fallback(). This could render the excess ETH stuck in the contract forever because primitiveOutputAmount() handles a balance check that calculates the exact amount of tokens received in the Curve operation. The same shall apply to other token types inadvertently received by the contract.

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/CurveTricryptoAdapter.sol#L223

        uint256 rawOutputAmount = _getBalance(underlying[outputToken]) - _balanceBefore;

I recommend having a function the owner can make withdraws on the stuck excess funds when needed.

[L-11] Risk assessment and mitigation strategies for WBTC depeg

According to the contract NatSpec of CurveTricryptoAdapter.sol, the Curve tricrypto adapter has WBTC in the trio pool:

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/CurveTricryptoAdapter.sol#L21-L24

/**
 * @notice
 *   curve tricrypto adapter contract enabling swapping, adding liquidity & removing liquidity for the curve usdt-wbtc-eth pool
 */

It's noteworthy that the potential depegging of WBTC (Wrapped Bitcoin) from Bitcoin poses a risk in the DeFi ecosystem, as evidenced by past instances like the November 2021 event linked to the FTX collapse, with its price deviating from its native asset, Bitcoin. This was primarily attributed to the activities and financial troubles of significant holders like Alameda Research, and other related factors contributing to market uncertainty and fear. During this period, market data indicated an 8.82% decrease in WBTC supply and a trading discount of 1.3% compared to Bitcoin. Such events raise concerns about the stability of wrapped assets and their reliance on the credibility of issuers like BitGo

"Analyzing the WBTC FUD after the FTX collapse and its depeg"

To mitigate this, it's recommended to regularly refactor and audit smart contract code, implement dynamic slippage adjustments, improve oracles for accurate price feeds, monitor liquidity pool health, develop circuit breakers and emergency protocols, educate the community about risks, encourage diversification in portfolios, and enhance governance mechanisms in DeFi platforms. These steps are crucial for managing risks associated with the depegging of wrapped assets like WBTC and maintaining stability in the DeFi market.

[L-12] Unrestricted asset approval

Curve2PoolAdapter._approveToken() and CurveTricryptoAdapter._approveToken() use type(uint256).max to set the maximum possible allowance for ERC-20 token transfers, a common practice in Ethereum smart contracts.

https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/Curve2PoolAdapter.sol#L189-L192 https://github.com/code-423n4/2023-11-shellprotocol/blob/main/src/adapters/CurveTricryptoAdapter.sol#L241-L244

    function _approveToken(address tokenAddress) private {
        IERC20Metadata(tokenAddress).approve(ocean, type(uint256).max);
        IERC20Metadata(tokenAddress).approve(primitive, type(uint256).max);
    }

This approach allows another address (ocean and primitive) to spend tokens on the user's behalf without repeatedly setting allowances, enhancing convenience and gas efficiency. However, it also carries a security risk; if the contract or approved address is compromised, an attacker could potentially access the user's entire token balance. While this method is prevalent, especially in decentralized finance (DeFi) applications, for its user-friendliness and efficiency, it is important to balance these benefits against the potential security vulnerabilities it introduces.

[NC-01] Incorrect comments

Ocean.forwardedDoInteraction() is making call to _doInteraction() instead of _doMultipleInteractions().

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

    function forwardedDoInteraction(
        Interaction calldata interaction,
        address userAddress
    )
        external
        payable
        override
        onlyApprovedForwarder(userAddress)
        returns (uint256 burnId, uint256 burnAmount, uint256 mintId, uint256 mintAmount)
    {
        emit ForwardedOceanTransaction(msg.sender, userAddress, 1);
        return _doInteraction(interaction, userAddress);
    }

As such, I recommend having the comments corrected as follows to avoid confusing readers:

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

-     * @dev call to _doMultipleInteractions() forwards the userAddress
+     * @dev call to _doInteractions() forwards the userAddress

[NC-02] Typo errors

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

- *  external contracts at certain points in its exection. The lifecycle is
+ *  external contracts at certain points in its execution. The lifecycle is

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

-        // mint before making a external call to the primitive to integrate with external protocol primitive adapters
+        // mint before making an external call to the primitive to integrate with external protocol primitive adapters

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

-        // burn before making a external call to the primitive to integrate with external protocol primitive adapters
+        // burn before making a external call to the primitive to integrate with external protocol primitive adapters

[NC-03] Activate the optimizer

Before deploying your contract, activate the optimizer when compiling using “solc --optimize --bin sourceFile.sol”. By default, the optimizer will optimize the contract assuming it is called 200 times across its lifetime. If you want the initial contract deployment to be cheaper and the later function executions to be more expensive, set it to “ --optimize-runs=1”. Conversely, if you expect many transactions and do not care for higher deployment cost and output size, set “--optimize-runs” to a high number.

module.exports = {
solidity: {
version: "0.8.20",
settings: {
  optimizer: {
    enabled: true,
    runs: 1000,
  },
},
},
};

Please visit the following site for further information:

https://docs.soliditylang.org/en/v0.5.4/using-the-compiler.html#using-the-commandline-compiler

Here's one example of instance on opcode comparison that delineates the gas saving mechanism:

for !=0 before optimization
PUSH1 0x00
DUP2
EQ
ISZERO
PUSH1 [cont offset]
JUMPI

after optimization
DUP1
PUSH1 [revert offset]
JUMPI

Disclaimer: There have been several bugs with security implications related to optimizations. For this reason, Solidity compiler optimizations are disabled by default, and it is unclear how many contracts in the wild actually use them. Therefore, it is unclear how well they are being tested and exercised. High-severity security issues due to optimization bugs have occurred in the past . A high-severity bug in the emscripten -generated solc-js compiler used by Truffle and Remix persisted until late 2018. The fix for this bug was not reported in the Solidity CHANGELOG. Another high-severity optimization bug resulting in incorrect bit shift results was patched in Solidity 0.5.6. Please measure the gas savings from optimizations, and carefully weigh them against the possibility of an optimization-related bug. Also, monitor the development and adoption of Solidity compiler optimizations to assess their maturity.