Should it transfer underSpentAmount to _msgSender() instead of reserve?

[code423n4]

Lines of code

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

Vulnerability details

Impact

underSpentAmount is dust amount, if being transferred to reserve without any state modification. These tokens might be lost. Normally, dust amount must transfer back to only _msgSender() as a best practice.

Proof of Concept

            unchecked {
                uint256 underSpentAmount = _inputTokenAmount - amountSpent;
                if (underSpentAmount != 0) {
                    SafeERC20.safeTransfer(_inputToken, address(reserve), underSpentAmount);
                }
                _decreaseHoldingAmount(_nftId, address(_inputToken), _inputTokenAmount - underSpentAmount);
            }

You will see that it is transferring underSpentAmount to reserve without any state modification.

Since it is dust amount, Normally, dust amount must transfer back to only _msgSender() as a best practice.

At least, if you want to force transfer to reserve, you should use _transferToReserveAndStore which transfer and update reserve state.

Tools Used

Manual

Recommended Mitigation Steps

You should transfer dust amount back to sender without any fee

            unchecked {
                uint256 underSpentAmount = _inputTokenAmount - amountSpent;
                if (underSpentAmount != 0) {
                    SafeERC20.safeTransfer(_inputToken, _msgSender(), underSpentAmount);
                }
                _decreaseHoldingAmount(_nftId, address(_inputToken), _inputTokenAmount - underSpentAmount);
            }

[obatirou]

Should it transfer underSpentAmount to _msgSender() instead of reserve? (disputed)

Since the factory use decreaseHoldingAmount with the amount - dust, dust continues to belong to the msgSender, but are stored in the NestedReserve