Tokens can get stuck in _submitOutOrders

[code423n4]

Handle

defsec

Vulnerability details

Impact

When dealing with ERC721 (instead of 1155) the amounts array is ignored, which leads to an issue.

User can call NestedFactory._submitOutOrders for an ERC721 with _inputTokenAmounts[i] = 0. The ERC721.transferFrom is still executed but user cannot _submitOutOrders later and the NFT is stuck as it checks (_inputTokenAmounts[i] > 0).

Proof of Concept

Tokens can get stuck. Also, subscribers to Request event could be tricked by specifying amounts[i] > 1 in the ERC721 case, as only one token was transferred but the amount multiple quantities get logged.

Tools Used

Code Review

Recommended Mitigation Steps

Check amounts[i] == 1 in ERC721 case, amounts[i] > 0 in 1155 case.

[maximebrugel]

The amount is the tokenId, so the user must be the owner of the token 0.

function transferFrom(
        address from,
        address to,
        uint256 tokenId
    ) public virtual override {
    //solhint-disable-next-line max-line-length
    require(_isApprovedOrOwner(_msgSender(), tokenId), "ERC721: transfer caller is not owner nor approved");

    _transfer(from, to, tokenId);
}

I don't know if I understand well the issue.

However, the protocol is focusing on ERC20, we are not supporting ERC721 and ERC1155, even if it can work in some cases.

[alcueca]

Dispute accepted. I saw nowhere that the protocol was expected to put ERC721 or ERC1155 into a NestedNFT.