Using LensBaseERC721::_safeTransfer may lead to tokens being locked forever

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/base/LensBaseERC721.sol#L475

Vulnerability details

Impact

As we are all aware of, the OZ Address.sol function isContract can be bypassed because (as the docs says):

It is unsafe to assume that an address for which this function returns false is an externally-owned account (EOA) and not a contract.

Among others, isContract will return false for the following types of addresses:

• an externally-owned account

• a contract in construction

• an address where a contract will be created

• an address where a contract lived, but was destroyed

so relying on such a function for critical operations like in all the functions which calls _safeTransfer directly or indirectly can lead to, for example, transferring an NFT to a recently destroyed contract (e. g. selfdestruct in the previous transaction due to block reordering), thus freezing that NFT forever (which is the main purpose of _safeTransfer removing that possibility)

// from https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/base/LensBaseERC721.sol#L285C8-L286C85

@dev Safely transfers `tokenId` token from `from` to `to`, checking first that contract recipients
 * are aware of the ERC721 protocol to prevent tokens from being forever locked.

...

Proof of Concept

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/base/LensBaseERC721.sol#L302C5-L312C6

function _safeTransfer(
        address from,
        address to,
        uint256 tokenId,
        bytes memory _data
    ) internal virtual {
        _transfer(from, to, tokenId);
        if (!_checkOnERC721Received(from, to, tokenId, _data)) { <================ depends on .isContract()
            revert Errors.NonERC721ReceiverImplementer();
        }
    }

https://github.com/code-423n4/2023-07-lens/blob/cdef6ebc6266c44c7068bc1c4c04e12bf0d67ead/contracts/base/LensBaseERC721.sol#L469C5-L490C6

function _checkOnERC721Received(
        address from,
        address to,
        uint256 tokenId,
        bytes memory _data
    ) private returns (bool) {
        if (to.isContract()) {
            try IERC721Receiver(to).onERC721Received(msg.sender, from, tokenId, _data) returns (bytes4 retval) {
                return retval == IERC721Receiver.onERC721Received.selector;
            } catch (bytes memory reason) {
                if (reason.length == 0) {
                    revert Errors.NonERC721ReceiverImplementer();
                } else {
                    assembly {
                        revert(add(32, reason), mload(reason))
                    }
                }
            }
        } else {
            return true; <========================= HERE lack of deeper validation of to
        }
    }

Tools Used

Manual analysis

Recommended Mitigation Steps

Make sure, in the else clause, that there is a public key associated with the to address so that if there is not, reverts. The solution they provide is solid but it fails in that edge case, checking the pk in the else clause should be fine

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as low quality report

[Picodes]

What does "check there is a public key associated with the to address " mean?

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid