Wrapping ether with Ocean.doMultipleInteractions() reverts

[c4-bot-4]

Lines of code

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

Vulnerability details

Summary

Wrapping ether with doMultipleInteractions() reverts. This is the case if one or more of the interactions is handling ether. If we look at the internal helper function, _doMultipleInteractions(), it includes the following:

if (msg.value != 0) {
    balanceDeltas.increaseBalanceDelta(WRAPPED_ETHER_ID, msg.value);
    emit EtherWrap(msg.value, userAddress);
}

It checks that the msg.value is greater than 0 then successfully executes. The problem lies in the code block below:

// Execute the interactions
{
    Interaction memory interaction;
    address userAddress_ = userAddress;

    for (uint256 i = 0; i < interactions.length;) {
        interaction = interactions[i];

        (InteractionType interactionType, address externalContract) =
            _unpackInteractionTypeAndAddress(interaction);

        uint256 specifiedToken = _getSpecifiedToken(interactionType, externalContract, interaction);

        uint256 specifiedAmount;
        if (interaction.specifiedAmount == GET_BALANCE_DELTA) {
            specifiedAmount = balanceDeltas.getBalanceDelta(interactionType, specifiedToken);
        } else {
            specifiedAmount = interaction.specifiedAmount;
        }

        (uint256 inputToken, uint256 inputAmount, uint256 outputToken, uint256 outputAmount) =
        _executeInteraction(
            interaction, interactionType, externalContract, specifiedToken, specifiedAmount, userAddress_
        );

        if (inputAmount > 0) {
            balanceDeltas.decreaseBalanceDelta(inputToken, inputAmount);
        }

        if (outputAmount > 0) {
            balanceDeltas.increaseBalanceDelta(outputToken, outputAmount);
        }
        unchecked {
            ++i;
                }
        }
}

Here we’re doing the checks relevant to token-based interactions even if we aren’t dealing with any tokens. This becomes problematic because wrapping ether requires a specific Interaction struct as a placeholder, but this struct isn't associated with any member of the InteractionType enum. Consequently, when doMultipleInteractions() receives this struct with default values and attempts to use it in the internal call to _executeInteraction(), it triggers a reversion.

Impact

Users cannot wrap ether by calling doMultipleInteractions.

Proof of Concept

Alice wants to utilise Shell Protocol to wrap ether and an NFT. She calls doMultipleInteractions() but her transaction reverts. Here is a Foundry test that demonstrates this, add this contract to the test directory:

// SPDX-License-Identifier: UNLICENSED
pragma solidity 0.8.20;

import {Test} from "forge-std/Test.sol";
import {Ocean} from "../src/ocean/Ocean.sol";
import "../src/ocean/Interactions.sol";
import "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import "@openzeppelin/contracts/token/ERC721/extensions/ERC721Burnable.sol";
import "@openzeppelin/contracts/access/Ownable.sol";

/// @dev Deploy a mock NFT contract.

contract MockNFT is ERC721, ERC721Burnable, Ownable {
    uint256 private _nextTokenId;

    constructor()
        ERC721("MockNFT", "MNFT")
        Ownable()
    {}

    function _baseURI() internal pure override returns (string memory) {
        return "foo";
    }

    function safeMint(address to) public {
        uint256 tokenId = _nextTokenId++;
        _safeMint(to, tokenId);
    }
}

/// @dev Foundry test.

contract OceanTest is Test {
    Ocean ocean;
    MockNFT mockNFT;

    function setUp() public {
        ocean = new Ocean("foo");
        mockNFT = new MockNFT();
    }

    function test_DoMultipleInteractions() public {
        address alice = makeAddr("alice");

        vm.startPrank(alice);
        mockNFT.safeMint(alice);
        mockNFT.approve(address(ocean), 0);
        vm.stopPrank();

        bytes32 interactionTypeAndAddress = bytes32(uint256(InteractionType.WrapErc721) << 248) | bytes32(uint256(uint160(address(mockNFT))));

        Interaction memory wrapNft = Interaction({
            interactionTypeAndAddress: interactionTypeAndAddress,
            inputToken: 0,
            outputToken: 0,
            specifiedAmount: 1,
            metadata: bytes32(0)
        });

        Interaction memory placeholder = Interaction({
            interactionTypeAndAddress: bytes32(0),
            inputToken: 0,
            outputToken: 0,
            specifiedAmount: 0,
            metadata: bytes32(0)
        });

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

        interactions[0] = wrapNft;
        interactions[1] = placeholder;

        uint256[] memory ids = new uint256[](2);

        ids[0] = 51512269482688776817431318513247169522164487422735541505705966567794458871541;
        ids[1] = 68598205499637732940393479723998335974150219832588297998851264911405221787060;

        vm.deal(alice, 1 ether);

        vm.prank(alice);
        vm.expectRevert();
        ocean.doMultipleInteractions{value: 1 ether}(interactions, ids);
    }
}

forge test --match-path test/Ocean.t.sol

Notice that if we change test_DoMultipleInteractions() as follows, it passes and the NFT is successfully wrapped validating that it is in fact the wrapping ether interaction that causes the revert.

...
    function test_DoMultipleInteractions() public {
        address alice = makeAddr("alice");

        vm.startPrank(alice);
        mockNFT.safeMint(alice);
        mockNFT.approve(address(ocean), 0);
        vm.stopPrank();

        bytes32 interactionTypeAndAddress = bytes32(uint256(InteractionType.WrapErc721) << 248) | bytes32(uint256(uint160(address(mockNFT))));

        Interaction memory wrapNft = Interaction({
            interactionTypeAndAddress: interactionTypeAndAddress,
            inputToken: 0,
            outputToken: 0,
            specifiedAmount: 1,
            metadata: bytes32(0)
        });

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

        interactions[0] = wrapNft;

        uint256[] memory ids = new uint256[](1);

        ids[0] = 51512269482688776817431318513247169522164487422735541505705966567794458871541;

        vm.prank(alice);
        ocean.doMultipleInteractions(interactions, ids);
    }

Tools Used

• Manual Review
• Foundry

Recommended Mitigation Steps

Consider adding an else statement after the if statement. This way the code block relevant to token-based interactions is only executed if msg.value is 0, Ocean.sol#L474-L483:

if (msg.value != 0) {
    balanceDeltas.increaseBalanceDelta(WRAPPED_ETHER_ID, msg.value);
    emit EtherWrap(msg.value, userAddress);
-   }
+   } else

// Execute the interactions
{

Assessed type

Error

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #35

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Invalid