search

code-423n4 / 2021-11-nested-findings

1 stars 1 forks source link

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

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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.