Out of Bounds Error in uint comparison leads to false positives

[ghost]

Description

In Offers.sol, a require statement compares a uint256 to a uint128.

The Solidity compiler will therefore compare not only the actual 128 bits of the variable itself, but also the neighboring 128 bits (no matter what data there is there).

This will lead to false positives when the neighboring data contains any data.

Scenario

Every call to .fulfillOffer() suffers from this comparison.

The false positives will occur when data exists in the 128 neighboring bits to _minOfferPrice.

Since the neighboring 128 bits are _tokenId, they will always contain data, leading to false positives.

Impact

Medium - an attacker can bypass the minOfferPrice check. This check was originally put in place to prevent fulfilling offers that are lower than a seller would want to sell their token for. Thus, attackers can cause sellers to sell their token for lower than they would intend to sell them for.

Reproduction

Consider the following line of code in fulfillOffer():

require(offerPrice >= _minOfferPrice, "cannot fufill offer – offer price too low");

Notice that it compares a uint256 (offerPrice) to a uint128 (_minOfferPrice). Because of this, Solidity will also consider the neighboring 128 bits to _minOfferPrice when making the comparison, treating both sides of the comparison as uint256.

Notice that the neighboring 128 bits to _minOfferPrice are the first 128 bits of tokenId.

Since all tokenId's are greater than zero, these bits will always be non-zero, leading to comparison problems.

Fix

Change the type of the variable _minOfferPrice from uint128 to uint256. You can do this by changing the function declaration from this:

function fulfillOffer(uint256 _tokenId, uint128 _minOfferPrice) external whenNotFrozen  {...}

to this

function fulfillOffer(uint256 _tokenId, uint256 _minOfferPrice) external whenNotFrozen {...}

[sunsetlover]

I'm confused because if it does indeed use the 128 bits to the left of it (tokenID), wouldn't that just make _minOfferPrice larger? Not smaller? If that's true, still a bug of course.

Also, with remix and web3 to interact with a contract with an ABI, each input to a function takes up 256 bits, no matter what they're types actually are. So the input for token 100 and a min price of 100 would look like (of course first 10 characters depends on the deployed contract) 0xd519d94600000000000000000000000000000000000000000000000000000000000000640000000000000000000000000000000000000000000000000000000000000064, so the 128 bits to the right are all 0 and your issue wouldn't happen.

I'm guessing this is from using solidity as a compiler? Maybe other compilers would lead to issues like you're talking about. Do you know? I am curious.

[flockonus]

If i understand correctly, you are saying that Solidity would fill the 128b value with another 128b memory garbage and use that for comparison. I really don't believe this is the case, but willing to be proven wrong if you have a working example of that (in solidity + test)

[arthcmr]

Thanks for your participation, @michaelKim4736! Our team has reviewed your submission, and while we are grateful, we have decided it does not warrant a reward.