Passing multiple ETH deposits in orders array will use the same msg.value many times

[code423n4]

Handle

hyh

Vulnerability details

Impact

Contract holdings can be emptied as malicious user will do deposit/withdraw to extract value. This is possible because after _transferInputTokens system uses contract balance for user's operations, assuming that equivalent value was transferred.

Proof of Concept

msg.value persist over calls, so passing 'Order[] calldata _orders' holding multiple ETH deposits will use the same msg.value in each of them, resulting in multiple deposits, that sums up to much bigger accounted value than actually deposited value, up to contract's ETH holdings.

create / addTokens -> _submitInOrders -> _transferInputTokens
https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L103
https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L119

sellTokensToWallet -> _submitOutOrders -> _transferInputTokens
https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L172

sellTokensToNft -> _submitOutOrders -> _transferInputTokens
https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L152

_transferInputTokens uses msg.value:
https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L462

Recommended Mitigation Steps

Controlling ETH to be only once in orders will not help, as NestedFactory inherits from Multicall, which multicall(bytes[] calldata data) function allows same reusage of msg.value, which will persist over calls.

So, it is recommended to treat ETH exclusively, not allowing ETH operations to be batched at all.

[adrien-supizet]

Multicall is not currently used, and the funds exposed would be the NestedFactory's which should hold no funds.

To avoid future bugs, we're going to remove the multicall library, but we don't think this is a high severity issue.

[alcueca]

Downgrading severity to 2 because the NestedFactory is not expected to hold funds, and therefore there is no risk of a loss. You can't deposit the same Ether twice in the WETH contract.

Also keeping this as the main over #13.