Specifying specifiedAmount to max doesn't always get the right amount into account

[c4-bot-4]

Lines of code

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

Vulnerability details

When the user specifies a interaction he/she can set specifiedAmount to max in the intention to get the amount of desired token into account for un/wraping.

But this only works if the previous interaction has set this value before the next interaction gets it.

There can be a situation that will be describe in the POC when this doesn't work correctly.

Impact

The user cannot always relay on specifying specifiedAmount value to max to get the right amount.

Proof of Concept

The following POC will demonstrate what happens when the user tries to get/unwrap the output token in two separate interactions:

// First interaction

1. The user makes a swap to get some output token
2. The swap leaves the output token inside the ocean contract

// Second interaction

3. The user tries to unwrap the output token from the ocean contract by specifying the specifiedAmount to max but it fails and the token remain inside the ocean contract

[PASS] test_TwoInteractions() (gas: 699992)
Logs:
  Bound Result 1090581708999999997
  balance of output token inside the ocean contract before: 44861951827
  balance of output token inside the ocean contract after: 44861951827

    function test_TwoInteractions() public {
        // vm.startPrank(address(0x123));
        vm.startPrank(wallet);

        // unwrapFee = bound(unwrapFee, 2000, type(uint256).max);
        // ocean.changeUnwrapFee(unwrapFee);

        address inputAddress;
        address outputAddress;
        bool toggle = true;
        uint256 amount = 10e18;
        if (toggle) {
            inputAddress = wbtcAddress;
            outputAddress = usdtAddress;
            amount = bound(amount, 1e17, IERC20(inputAddress).balanceOf(wallet) * 1e9);
        } else {
            inputAddress = usdtAddress;
            outputAddress = wbtcAddress;
            amount = bound(amount, 1e17, IERC20(inputAddress).balanceOf(wallet) * 1e11);
        }

        IERC20(inputAddress).approve(address(ocean), amount);

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

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

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

        uint balance = IERC20(outputAddress).balanceOf(address(ocean));
        console.log("balance of output token inside the ocean contract before:", balance);
        // assertLt(newInputBalance, prevInputBalance);
        // assertGt(newOutputBalance, prevOutputBalance);

        Interaction[] memory interactions2 = new Interaction[](1);
        interactions2[0] = Interaction({
                interactionTypeAndAddress: _fetchInteractionId(outputAddress, uint256(InteractionType.UnwrapErc20)),
                inputToken: 0,
                outputToken: 0,
                specifiedAmount: type(uint256).max,
                metadata: bytes32(0)
            });
        ocean.doMultipleInteractions(interactions2, ids);

        console.log("balance of output token inside the ocean contract after:", IERC20(outputAddress).balanceOf(address(ocean)));

        vm.stopPrank();
    }

Tools Used

forge of foundry

Recommended Mitigation Steps

By modifying the method _doMultipleInteractions inside the ocean contract we can check the balance from the contract and adjust its decimals to get the correct balanceDelta amount :

From

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

to

    // A user can pass uint256.max as the specifiedAmount when they
    // want to use the total amount of the token held in the
    // balance delta. Otherwise, the specifiedAmount is just the
    // amount the user passed for this interaction.
    uint256 specifiedAmount;
    if (interaction.specifiedAmount == GET_BALANCE_DELTA) {
       specifiedAmount = balanceDeltas.getBalanceDelta(interactionType, specifiedToken);
+       if (specifiedAmount == 0) {
+           try IERC20Metadata(externalContract).decimals() returns (uint8 decimals) {
+           
+               if (decimals > 0) {
+                   specifiedAmount = IERC20(externalContract).balanceOf(address(this));
+                   (specifiedAmount,) = _convertDecimals(decimals, NORMALIZED_DECIMALS, specifiedAmount);
+               }
+               else {
+                   specifiedAmount = balanceDeltas.getBalanceDelta(interactionType, specifiedToken);
+               }
+   
+           } catch {
+               specifiedAmount = balanceDeltas.getBalanceDelta(interactionType, specifiedToken);
+           }
+       }
    } else {
        specifiedAmount = interaction.specifiedAmount;
    }

Also add the following line to the OceanAdapter abstract contract to help the code differentiate between adapters and tokens.

function decimals() external pure returns(uint256) { return 0; }

When we rerun the previous test we get:

[PASS] test_TwoInteractions() (gas: 670489)
Logs:
  Bound Result 1090581708999999997
  balance of output token inside the ocean contract before: 44861951827
  balance of output token inside the ocean contract after: 0

Assessed type

Token-Transfer

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #142

[c4-judge]

0xA5DF marked the issue as selected for report

[c4-judge]

0xA5DF marked issue #142 as primary and marked this issue as a duplicate of 142

[c4-judge]

0xA5DF marked the issue as not a duplicate

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Invalid

[0xA5DF]

But this only works if the previous interaction has set this value before the next interaction gets it.

Seems like intended design