Function NestedFactory.destroy doesn't check amounts returned after executing user's market order to sell a portfolio

[code423n4]

Handle

hyh

Vulnerability details

Impact

Malicious user can track market order and perform sandwich attack whenever order amount to be executed on an exchange is big enough to compensate for exchange manipulation costs.

Proof of Concept

User facing destroy function doesn't check amount returned after executing user's order, this way accepting any amount returned from market trade, thus is vulnerable.

https://github.com/code-423n4/2021-11-nested/blob/main/contracts/NestedFactory.sol#L227

Recommended Mitigation Steps

Returned amountBought should be checked against user provided minimum viable return amount, for example, minAmount. This amount to be added as another argument to the destroy function, which should check the amounts:

require(amountBought >= minAmount, "NestedFactory::_submitOutOrders: Slippage too big");

[adrien-supizet]

The trades occur on 0x currently, which allows specifying the slippage tolerance directly in each instance of Order.calldata. This way we ensure that each token is sold at a rate which the user has agreed with.

[alcueca]

Dispute accepted