search

code-423n4 / 2022-02-nested-findings

0 stars 0 forks source link

NestedFactory: User can utilise accidentally sent ETH funds via processOutputOrders() / processInputAndOutputOrders() #41

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L71 https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L286-L296 https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L370-L375 https://github.com/code-423n4/2022-02-nested/blob/main/contracts/NestedFactory.sol#L482-L492

Vulnerability details

Impact

Should a user accidentally send ETH to the NestedFactory, anyone can utilise it to their own benefit by calling processOutputOrders() / processInputAndOutputOrders(). This is possible because:

  1. receive() has no restriction on the sender
  2. processOutputOrders() does not check msg.value, and rightly so, because funds are expected to come from reserve.
  3. transferInputTokens() does not handle the case where ETH could be specified as an address by the user for an output order.
if (address(_inputToken) == ETH) {
  require(address(this).balance >= _inputTokenAmount, "NF: INVALID_AMOUNT_IN");
  weth.deposit{ value: _inputTokenAmount }();
  return (IERC20(address(weth)), _inputTokenAmount);
}

Hence, the attack vector is simple. Should a user accidentally send ETH to the contract, create an output Order with token being ETH and amount corresponding to the NestedFactory’s ETH balance.

Recommended Mitigation Steps

  1. Since plain / directETH transfers are only expected to come from weth (excluding payable functions), we recommend restricting the sender to be weth, like how it is done in [FeeSplitter](https://github.com/code-423n4/2022-02-nested/blob/main/contracts/FeeSplitter.sol#L101-L104). Also, from what we see, plain ETH transfers are also not expected to come from other sources like NestedReserve or operators.
receive() external payable {
  require(msg.sender == address(weth), "NF: ETH_SENDER_NOT_WETH");
}

P.S we are aware that this was raised previously here: https://github.com/code-423n4/2021-11-nested-findings/issues/188 and would like to add that you can have payable functions without the existence of the receive() function.

  1. Check that _fromReserve is false in the scenario address(_inputToken) == ETH.
if (address(_inputToken) == ETH) {
    require(!_fromReserve, "NF: INVALID_INPUT_TOKEN");
    require(address(this).balance >= _inputTokenAmount, "NF: INVALID_AMOUNT_IN");
    weth.deposit{ value: _inputTokenAmount }();
    return (IERC20(address(weth)), _inputTokenAmount);
}
maximebrugel commented 2 years ago

Duplicated : https://github.com/code-423n4/2022-02-nested-findings/issues/44

CloudEllie commented 2 years ago

Warden requested that this issue be withdrawn in favour of the updated one ( #44 )