Naming collision between global and local variables in cancelOffer and fulfillOffer

[ghost]

Description

OffersConfig.sol defines a global variable named uint256 offerCut.

However, Offers.sol then tries to define a new local variable called uint256 offerCut even though the global variable of the same name is still in scope.

The global variable is defined in OffersConfig.sol, and the local variable is defined in both .fulfillOrder() and in .cancelOrder().

Scenario

Every call to .cancelOffer() and .fulfillOffer() trigges this naming collision. Consider the line

uint256 public offerCut;

in OffersConfig.sol, and compare it to the line

 uint256 offerCut = uint256(offer.offerCut);

in both .fulfillOrder() and in .cancelOrder().

Impact

Medium - if Solidity does not allow naming collisions, it leads to a runtime error. If Solidity does allow for naming collisions, it leads to developer's confusion in potentially referencing or modifying the wrong variable.

Reproduction

Notice the line uint256 offerPrice = _computeOfferPrice(total, offerCut); in .fulfillOrder() at line 224. If Solidity interprets offerCut as being the variable defined on the line before, the calculation will proceed correctly. However, if Solidity interprets offerCut to be the global variable that already exists in scope, then ._computeOfferPrice() will produce an incorrect result. In either case, developers will be confused later on in the function when referring to offerCut, and may potentially call the wrong one.

Fix

Rename uint256 offerCut in both .fulfillOrder() and .cancelOrder() to avoid naming collision.

[hwrdtm]

Thanks @TomLeeFounder for your feedback! We will take it into consideration.

[arthcmr]

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