Passing multiple ETH deposits in orders array will use the same msg.value many times
Opened this issue · 2 comments
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.
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.