ERC721_transferFrom and ERC721_safeTransferFrom allow improper transfer of tokens

[smonicas]

Hi,

I work at Trail of Bits and I noticed a critical bug recently introduced in the ERC721 support: an incorrect handling of the zero address in ERC721_transferFrom / ERC721_safeTransferFrom allows anyone to steal the tokens.

cairo-contracts/contracts/token/ERC721_base.cairo

Lines 183 to 194 in 87d6d81

	func ERC721_transferFrom{
	pedersen_ptr: HashBuiltin*,
	syscall_ptr: felt*,
	range_check_ptr
	}(_from: felt, to: felt, token_id: Uint256):
	let (caller) = get_caller_address()
	let (is_approved) = _is_approved_or_owner(caller, token_id)
	assert is_approved = 1
	_transfer(_from, to, token_id)
	return ()
	end

cairo-contracts/contracts/token/ERC721_base.cairo

Lines 196 to 213 in 87d6d81

	func ERC721_safeTransferFrom{
	pedersen_ptr: HashBuiltin*,
	syscall_ptr: felt*,
	range_check_ptr
	}(
	_from: felt,
	to: felt,
	token_id: Uint256,
	data_len: felt,
	data: felt*
	):
	let (caller) = get_caller_address()
	let (is_approved) = _is_approved_or_owner(caller, token_id)
	assert is_approved = 1
	_safe_transfer(_from, to, token_id, data_len, data)
	return ()
	end

In cairo, calling a function directly (i.e, without going through an account contract) lead the message’s sender to be zero.

ERC721_getApproved returns the address approved:

cairo-contracts/contracts/token/ERC721_base.cairo

Lines 109 to 119 in 87d6d81

	func ERC721_getApproved{
	syscall_ptr: felt*,
	pedersen_ptr: HashBuiltin*,
	range_check_ptr
	}(token_id: Uint256) -> (approved: felt):
	let (exists) = _exists(token_id)
	assert exists = 1
	let (approved) = ERC721_token_approvals.read(token_id)
	return (approved)
	end

However if no address was approved, this will return the zero address (as storage is initialized to 0).

As a result, anyone calling ERC721_transferFrom / ERC721_safeTransferFrom directly will be approved, and can steal any tokens.

A short-term fix is for ERC721_transferFrom and ERC721_safeTransferFrom to not accept the zero address as caller. However, this will prevent anyone from using these functionalities without going through a contract.

[andrew-fleming]

Hey, thank you so much for pointing this out! A prompt fix is forthcoming.

[martriay]

However, this will prevent anyone from using these functionalities without going through a contract.

Do you see this as a problem? I think once fees are mandatory there will be no way to interact directly with contracts, unless they're implemented explicitly to sponsor user interaction in which case they can also handle this.

[smonicas]

Do you see this as a problem?

No not a problem just a consideration.