add overflow check on _minOfferPrice in fulfillOffer

[sunsetlover]

Description

Because of overflow errors, a user could accept an offer much lower than they want. It is possible although probably low risk but potentially high impact.

Scenario

A user calls fulfillOffer with a _minOfferPrice that is larger than uint128. This could happen purely on accident by the user, or potentially intentionally or accidentally by a third party application the user is using.

Because _minOfferPrice is larger than uint128, fulfillOffer could potentially treat _minOfferPrice as a very small number close to 0 (if say _minOfferPrice = 2^128 +1). Then

In the meantime before the user's tx is mined, an attacker could perform a frontrunning attack.

He or she could see that this user will end up with an overflow error then

1. outbid the current offer
2. cancel their offer and
3. submit a low offer that will be accepted by the overflow error.

Impact

A user could essentially give away a high valued cat for a very small amount of money.

Reproduction

See my comment below: #33 (comment)

Fix

In fulfillOffer, add the following line in the beginning: require(_minOfferPrice == uint256(uint128(_minOfferPrice)))

[ghost]

How can a user call fulfillOffer with a _minOfferPrice that is larger than uint128? The second argument is correctly typed as uint128 in the function declaration, so it would only accept inputs of type uint128, see the following snippet:

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

[sunsetlover]

I wrote a simple contract with remix that had this function function tester(uint8 x) public returns (uint32){holder = x;} and a state variable called holder initialized to 0.

With remix, if I call tester(500), it changes holder to 244 = 500 - 256 (255 is the largest 8 bit number).

If I have time, I can launch a contract and see if I get this behavior on EVM. I think the issue is that every input to a function (at least in remix and I believe if you use web3 and a ABI to interact with a contract) takes 256 bits--regardless of what it "should" be.

Also, on the main CK contract, it is pretty standard to check input types are what they should be (however just realized this is in a different context: the input is allowed to be 256 bits, but they require it is truly 128 bits). See lines 1424 - 1428 here: https://etherscan.io/address/0x06012c8cf97bead5deae237070f9587f8e7a266d#code

[sunsetlover]

Also, I just realized that the impact of this issue and #27 are the same (although my issue is far more unlikely). However, the reason for the bug is different. Also, not to say they are wrong, but personally I am not able to reproduce their bug with remix.

[sunsetlover]

I still do not agree with you. This is a real issue that can happen. I just launched a contract:

contract type_test {
    
    address OWNER = 0x8366d93c271c37504ca0bf3c06d40aa76251d21b;
    
    uint256 public holder = 0;
    
    function overflow_test(uint8 x){
        holder = x;
    }
   
    function destroy_me() external {
        require(msg.sender == OWNER);
        selfdestruct(this);
    }
}

Here's the address: https://etherscan.io/address/0x78734a93c98514fa5f605463a7f1efbf9d88e29e

Unfortunately, I am having trouble getting etherscan to publish the code of the contract. But anyway, I called overflow_test(500) (here:https://etherscan.io/tx/0x6b5c6662e56a011815c4f79c45715714faec74515dc3895cc8102d6fc98edc73) and as I saw on remix, it changed the value of holder to 244. As you can see, the input that was sent to the EVM was indeed 500-- which is clearly not a uint8--and it did not fail.

Here's is the abi if you want to test yourself:
var abi = [{"constant":false,"inputs":[{"name":"x","type":"uint8"}],"name":"overflow_test","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":false,"inputs":[],"name":"destroy_me","outputs":[],"payable":false,"stateMutability":"nonpayable","type":"function"},{"constant":true,"inputs":[],"name":"holder","outputs":[{"name":"","type":"uint256"}],"payable":false,"stateMutability":"view","type":"function"}]

The point is depending on the compiler or perhaps the way you interact with the contract, the input can be uint256 even if it's not "supposed" to. Nothing is enforcing that the correct uint type is getting passed into functions.

[sunsetlover]

A similar situation arises for some global variables (wanted to leave this as a comment since it seemed unnecessary to open an entire new bug report since it is somewhat similar):

Description

There are potential functionality issues due to overflow errors because of type casting variables as uint128 or uint64 which are composed of a sum of variables that include either uint256 variables globalDuration.

(Note as far as I am aware, the following global variables are safe with respect to overflow errors: minimumTotalValue (because it is never invovled with something that gets type cast) and minimumPriceIncrement / offerCut
(because it gets checked if its below 10k even at contract creation).)

Scenario

For globalDuration: COO sets globalDuration to a value larger than uint64 (most likely by accident since any reasonable time would fit in a uint64. I suppose a really large number could be used as a proxy for saying the offers never expire).

When someone calls createOffer() or updateOffer(), in lines 114 and 332 respectively, the expiration time of the offer gets type cast to uint64 for the Offers struct. However, in this case, there will be an underflow error meaning that the intended expiration time of the offer is much less.

Impact

Low likelihood and low impact (someone could lose a small amount of fees if the auction only lasts a few minutes).

Fix

Add a line like require(globalDuration == uint256(uint128(globalDuration)) whenever globalDuration is changed.

[hwrdtm]

Thanks @sunsetlover for your feedback! We will take this into consideration

[arthcmr]

Thanks for your participation, @sunsetlover! Our team has reviewed your submission, and we are pleased to reward you for your report.

Severity: Note
Points: 50