The unwrapETH2LD use transferFrom instead of safeTransferFrom to transfer ERC721 token

[code423n4]

Lines of code

https://github.com/code-423n4/2022-07-ens/blob/ff6e59b9415d0ead7daf31c2ed06e86d9061ae22/contracts/wrapper/NameWrapper.sol#L327-L346

Vulnerability details

Impact

The unwrapETH2LD use transferFrom to transfer ERC721 token, the newRegistrant could be an unprepared contract

Proof of Concept

Should a ERC-721 compatible token be transferred to an unprepared contract, it would end up being locked up there. Moreover, if a contract explicitly wanted to reject ERC-721 safeTransfers.
Plus take a look to the OZ safeTransfer comments;
Usage of this method is discouraged, use safeTransferFrom whenever possible.

Tools Used

Manual Review

Recommended Mitigation Steps

    function unwrapETH2LD(
        bytes32 labelhash,
        address newRegistrant,
        address newController
    ) public override onlyTokenOwner(_makeNode(ETH_NODE, labelhash)) {
        _unwrap(_makeNode(ETH_NODE, labelhash), newController);
-       registrar.transferFrom(
+       registrar.safeTransferFrom(
            address(this),
            newRegistrant,
            uint256(labelhash)
        );
    }

[jefflau]

transfer is to the contract itself, so there is no point in using safeTransferFrom. For other situations where transferFrom the behaviour is intended.

[dmvt]

transfer is to the contract itself, so there is no point in using safeTransferFrom. For other situations where transferFrom the behaviour is intended.

That's incorrect in the report above. This is transferring from, not to, the contract itself.

[jefflau]

That's incorrect in the report above. This is transferring from, not to, the contract itself.

Yes sorry, that is true. I was replying to some of the duplicates that I closed such as: #126 #147