Nested Finance contest - 0xliumin's results

Lines of code

https://github.com/code-423n4/2022-02-nested/blob/69cf51d8e4eeb8bce3025db7f4f74cc565c9fad3/contracts/NestedFactory.sol#L446

Vulnerability details

Impact

A user can destroy their NFTs and not pay fees on most of their assets.

Proof of concept

Alice has an NFT portfolio with 100 gwei dai and 100 gwei uni. Alice calls destroy on this NFT with buy token marked as dai. We would expect after this destroy step that she would pay 1 gwei dai in fees + 1 gwei uni worth of dai in fees, no matter what orders she selects.

Alice selects the following orders:

[{ operator: ZeroEx, token: uni, calldata: performSwap with a very small amount of uni for dai}
 { operator: Flat, token: dai, calldata: transfer a very small amount dai for dai}]

This set of orders doesn't violate any of the require statements in the destroy function. Each order will complete successfully given the constraints here: https://github.com/code-423n4/2022-02-nested/blob/69cf51d8e4eeb8bce3025db7f4f74cc565c9fad3/contracts/abstracts/MixinOperatorResolver.sol#L100-L101

Fees aren't collected on the leftover amount from the callOperator step, seen here https://github.com/code-423n4/2022-02-nested/blob/69cf51d8e4eeb8bce3025db7f4f74cc565c9fad3/contracts/NestedFactory.sol#L446

this means that Alice will only pay the fees on the dai that was output from the orders (a very small amount), and the rest of the 100 gwei uni and 100 gwei dai are transferred directly back to the attacker.

Recommended mitigation

Replace the safeTransfer with your safeTransferWithFees function. Or, if you don't want users to have to pay fees on slippage, set a maximum "slippage" amount in the safeSubmitOrder function.

Overall the code is high-quality and easy to follow. I did find some low-risk and non-critical findings below.

Low risk findings

areOperatorsImported has incorrect logic

Right now, this function returns false if the implementation AND the selector don't match. It's possible to provide a destination with either a different implementation or selector, and still get a true response.

https://github.com/code-423n4/2022-02-nested/blob/69cf51d8e4eeb8bce3025db7f4f74cc565c9fad3/contracts/OperatorResolver.sol#L42 Mitigation is to change && to ||.

Accidentally calling withdraw twice with the same parameters could withdraw multiple assets

If a user accidentally submits two withdraw transactions from the frontend, they will withdraw the token at that index and the last token in the list originally. Making sure that the user is unable to do this on the frontend will be important.

Non-critical findings

Flat operator doesn't need IERC20 imported from openzeppelin

No nftDestroyed event on destroy call

No nftUpdated event on updateLockTimestamp function