Lines of code

Vulnerability details

Impact

Some ERC-20 tokens do not revert on failure (such as ZRX). Instead, they would just return a boolean false. In function NeoTokyoStaker._assetTransferFrom(), the check success is only checking whether the low-level call is successful or not. If the ERC-20 token only returns a boolean, success will be true even if the transfer fails.

#Proof of Concept

/**
	A private helper function for performing the low-level call to 
	transferFrom on either a specific ERC-721 token or some amount of ERC-20 
	tokens.

	@param _asset The address of the asset to perform the transfer call on.
	@param _from The address to attempt to transfer the asset from.
	@param _to The address to attempt to transfer the asset to.
	@param _idOrAmount This parameter encodes either an ERC-721 token ID or an 
		amount of ERC-20 tokens to attempt to transfer, depending on what 
		interface is implemented by _asset.
*/
function _assetTransferFrom (
	address _asset,
	address _from,
	address _to,
	uint256 _idOrAmount
) private {
	(bool success, bytes memory data) = 
		_asset.call(
			abi.encodeWithSelector(
				_TRANSFER_FROM_SELECTOR,
				_from,
				_to, 
				_idOrAmount
			)
		);

	// Revert if the low-level call fails.
	if (!success) {
		revert(string(data));
	}
}

As mentioned in “Weird ERC20 Tokens”:

> Some tokens do not revert on failure, but instead return false (e.g. ZRX).
>
> While this is technically compliant with the ERC20 standard, it goes against common solidity coding practices and may be overlooked by developers who forget to wrap their calls to transfer in a require.

The bool success on line 772 checks if the low-level call is successful. It does not check the status of transfer() (called via selector _TRANSFER_FROM_SELECTOR). In the case of “no-revert-on-failure” tokens, if the low-level call succeeded, bool success will always return true regardless whether transfer() succeeds or not.

Tools Used

Manual review.

Recommended Mitigation Steps

Create a function selector _SAFE_TRANSFER_FROM_SELECTOR and use it in NeoTokyoStaker._assetTransferFrom().

The text was updated successfully, but these errors were encountered:

All reactions