oceanAdapter.computeOutputAmount() fails to calculate the correct value for unwrappedAmount and thus the wrong value for outputAmount.

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. oceanAdapter.computeOutputAmount() fails to calculate the correct value for unwrappedAmount and thus will calculate the wrong value for outputAmount.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

oceanAdapter.computeOutputAmount() unwraps the input tokens and then calcualtes the outputAmount and finally wraps the output tokens.

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

However, the problem is that unwrapToken() considers the unwrapping fee as follows. It considers both the percentgage using function _calculateUnwrapFee() but also the truncated amount. In other words, the actual fee charge is amount/unwrapFeeDivisor + truncated, not amount/unwrapFeeDivisor!

 function _erc20Unwrap(address tokenAddress, uint256 amount, address userAddress, uint256 inputToken) private {
        try IERC20Metadata(tokenAddress).decimals() returns (uint8 decimals) {
            uint256 feeCharged = _calculateUnwrapFee(amount);
            uint256 amountRemaining = amount - feeCharged;

            (uint256 transferAmount, uint256 truncated) =
                _convertDecimals(NORMALIZED_DECIMALS, decimals, amountRemaining);
            feeCharged += truncated;

            _grantFeeToOcean(inputToken, feeCharged);

            SafeERC20.safeTransfer(IERC20(tokenAddress), userAddress, transferAmount);
            emit Erc20Unwrap(tokenAddress, transferAmount, amount, feeCharged, userAddress, inputToken);
        } catch {
            revert NO_DECIMAL_METHOD();
        }
    }

Meanwhile, oceanAdapter.computeOutputAmount() fails to account the amount in truncated. As a result, the unwrapFee is smaller than it is supposed to be and the unwrappedAmount will be larger than it is supposed to be. Finally, the outputAmount will be calculated wrongly.

  // handle the unwrap fee scenario
        uint256 unwrapFee = inputAmount / IOceanInteractions(ocean).unwrapFeeDivisor();
        uint256 unwrappedAmount = inputAmount - unwrapFee;          // wrong unwrapped amount...abi
        console2.log("unwapped amount", unwrappedAmount);

        outputAmount = primitiveOutputAmount(inputToken, outputToken, unwrappedAmount, metadata);

        wrapToken(outputToken, outputAmount);

The following POC confirms my finding: 1) There are three interactions. 2) The second interaction will eventually calls OceanAdaptor.computeOutputAmount() to unwrap 123456123456789012 input tokens from ocean.
3) Although only 123443 (6 decimals) = 123443000000000000 (18 decimals) wrapped usdc is unwrapped, the unwrappedAmount records a value of 123443777844443334, which is larger than is is supposed to be (123443000000000000).

pragma solidity 0.8.20;

import "forge-std/Test.sol";
import "forge-std/console.sol";
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "../../ocean/Interactions.sol";
import "../../ocean/Ocean.sol";
import "../../adapters/Curve2PoolAdapter.sol";

contract TestCurve2PoolAdapter is Test {
    Ocean ocean;
    address wallet = 0x9b64203878F24eB0CDF55c8c6fA7D08Ba0cF77E5; // USDC/USDT whale
    address lpWallet = 0x641D99580f6cf034e1734287A9E8DaE4356641cA; // 2pool LP whale
    Curve2PoolAdapter adapter;
    address usdcAddress = 0xFF970A61A04b1cA14834A43f5dE4533eBDDB5CC8;
    address usdtAddress = 0xFd086bC7CD5C481DCC9C85ebE478A1C0b69FCbb9;

    function setUp() public {
        vm.createSelectFork("https://arb1.arbitrum.io/rpc"); // Will start on latest block by default
        vm.prank(wallet);
        ocean = new Ocean("");
        adapter = new Curve2PoolAdapter(address(ocean), 0x7f90122BF0700F9E7e1F688fe926940E8839F353); // 2pool address
    }

    function testMyDeposit() public{
        uint256 unwrapFee = 10000;
        vm.startPrank(wallet);
        console2.log("wallet:", wallet);
        ocean.changeUnwrapFee(unwrapFee);
        assertEq(ocean.unwrapFeeDivisor(), 10000); // 1/10000

        address inputAddress = usdcAddress;
        address outputAddress = adapter.primitive();         // lpToken
        uint256 amount = 123456123456789012; // 100M , $1, => 1000,000
        IERC20(inputAddress).approve(address(ocean), amount);

        uint256 prevInputBalance = IERC20(inputAddress).balanceOf(wallet);
        uint256 prevOutputBalance = IERC20(outputAddress).balanceOf(wallet);

        console2.log("inputAddress:", inputAddress);
        console2.log("outputAddress: ", outputAddress);
        console2.log("inputAddress balance: %d ", prevInputBalance);
        console2.log("outputAddress balance: %d", prevOutputBalance);

        Interaction[] memory interactions = new Interaction[](3);

        interactions[0] = Interaction({
            interactionTypeAndAddress: _fetchInteractionId(inputAddress, uint256(InteractionType.WrapErc20)),
            inputToken: 0,
            outputToken: 0,
            specifiedAmount: amount,
            metadata: bytes32(0)
        });

        interactions[1] = Interaction({
            interactionTypeAndAddress: _fetchInteractionId(address(adapter), uint256(InteractionType.ComputeOutputAmount)),
            inputToken: _calculateOceanId(inputAddress),
            outputToken: _calculateOceanId(outputAddress),
            specifiedAmount: type(uint256).max,
            metadata: bytes32(0)
        });

        interactions[2] = Interaction({
            interactionTypeAndAddress: _fetchInteractionId(outputAddress, uint256(InteractionType.UnwrapErc20)),
            inputToken: 0,
            outputToken: 0,
            specifiedAmount: type(uint256).max,
            metadata: bytes32(0)
        });

        // erc1155 token id's for balance delta
        uint256[] memory ids = new uint256[](2);
        ids[0] = _calculateOceanId(inputAddress);
        ids[1] = _calculateOceanId(outputAddress);

        ocean.doMultipleInteractions(interactions, ids);

        uint256 newInputBalance = IERC20(inputAddress).balanceOf(wallet);
        uint256 newOutputBalance = IERC20(outputAddress).balanceOf(wallet);

        console2.log("New inputAddress balance: ", newInputBalance);
        console2.log("New outputAddress balance:", newOutputBalance); 

        vm.stopPrank();
    }
}

## Tools Used
VSCode

## Recommended Mitigation Steps
Record the actualy unwrap fee accurately and the correct unwrappedAmount.

## Assessed type

Math

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as high quality report

[c4-sponsor]

viraj124 (sponsor) acknowledged

[viraj124]

we'll add the update soon

[c4-judge]

0xA5DF marked the issue as satisfactory

[c4-judge]

0xA5DF marked the issue as selected for report

[0xA5DF]

I'm thinking of downgrading this to low, even for the low-decimals tokens (6 decimals) this is only 1e-6 of the 'traffic' in the contract. Meaning for each 1M USD worth of tokens passing through the function the protocol would lose 1 USD.

What do you think @viraj124? Is this an amount you'd consider meaningful? How much is the regular fee (without truncation) expected to be?

[viraj124]

we have anywys fixed it and I feel we can mark it as low @0xA5DF the fee on ocean v2 is negligible

[0xA5DF]

Got it, mitigation doesn't change the initial severity, but since the amount seems insignificant I'll downgrade it to low

[c4-judge]

0xA5DF changed the severity to QA (Quality Assurance)

[c4-judge]

0xA5DF marked the issue as grade-c

[0xA5DF]

Closing due to total low quantity of QA findings by warden