underSpentAmount should be transferred back to sender without any fee.

[code423n4]

Lines of code

https://github.com/code-423n4/2022-06-nested/blob/b4a153c943d54755711a2f7b80cbbf3a5bb49d76/contracts/NestedFactory.sol#L486-L505

Vulnerability details

Impact

underSpentAmount is dust amount, Currently, fee is even applied to dust amount, causing overspent user to loss some money due to this fee. Normally, dust amount must transfer back to only _msgSender() as a best practice without any fee applied to it.

Proof of Concept

            require(amounts[1] <= _amountToSpend, "NF: OVERSPENT");
            unchecked {
                uint256 underSpentAmount = _amountToSpend - amounts[1];
                if (underSpentAmount != 0) {
                    _safeTransferWithFees(IERC20(_inputToken), underSpentAmount, _msgSender(), _nftId);
                }
            }

You will see that you are correcting fee transferring underSpentAmount back to _msgSender().

Since these underSpentAmount is not being used, it should be transferred back without any fee.

Tools Used

Manual

Recommended Mitigation Steps

You should transfer dust amount back to sender without any fee

            require(amounts[1] <= _amountToSpend, "NF: OVERSPENT");
            unchecked {
                uint256 underSpentAmount = _amountToSpend - amounts[1];
                if (underSpentAmount != 0) {
                    SafeERC20.safeTransfer(IERC20(_inputToken), _msgSender(), underSpentAmount);
                }
            }

[obatirou]

underSpentAmount should be transferred back to sender without any fee. (disputed)

When we return the dust to the msgSender, we do the equivalent of a withdraw from the NestedReserve to the msgSender, so we must apply the fees corresponding to this withdrawal.

[jack-the-pug]

It's a best practice/suggestion, I'll downgrade it to QA.

[jack-the-pug]

This is indeed a misunderstanding of the design. I was wrong about this being a valid suggestion. It should be invalid.

The underSpentAmount is actually the amount that can not be converted to _buyToken (should be a dust amount in most cases, though), and the exitFee should be applied to this.

In the meantime, applying the exitFee onto the outputToken of the swap makes it possible for the users to bypass the exitFee by using malicious payloads for the swap. See #69.