Save gas by writing to storage once rather than twice in .updateOffer()

[ghost]

Description

.updateOffer() updates both the offer.total property and the offer.expiresAt property of an offer struct.

However, it writes each of these changes in two separate writes to storage.

We can save gas by making a single write to storage, rather than two.

Scenario

Every time that .updateOffer() is called to raise the price of an offer, we waste gas by writing to storage twice rather than once.

Impact

Medium: It costs 5,000 gas for each write to storage. If we do a single write to storage (and pack the properties more efficiently as mentioned in Issue #45), we can save 5,000 gas every single time the function is run.

Reproduction

(1) In .updateOffer(), we retrieve a pointer to the offer struct that we will update:

Offer storage offer = tokenIdToOffer[_tokenId];

(2) If msg.value is greater than zero, we update offer.total. Since this is a pointer to storage that we are updating, this costs 5,000 gas, a very expensive operation gas-wise:

offer.total += uint128(msg.value);

(3) Only after that do we update a different property of the same struct in storage, again costing us another 5,000 gas:

offer.expiresAt = uint64(newExpiresAt);

Fix

Instead, we can make both changes locally, and then do a single write to storage. If we combine this with the better struct-packing from Issue #45, we will save 5,000 gas on every write. Change this:

uint256 newExpiresAt = now + globalDuration;

// Check if the caller wants to raise the offer as well
if (msg.value > 0) {
    // Set the new price
    offer.total += uint128(msg.value);
}

offer.expiresAt = uint64(newExpiresAt);

to this:

uint256 newExpiresAt = now + globalDuration;

Offer updatedOffer = offer;

// Check if the caller wants to raise the offer as well
if (msg.value > 0) {
    // Set the new price
    updatedOffer.total += uint128(msg.value);
}

updatedOffer.expiresAt = uint64(newExpiresAt);

offer = updatedOffer;

We would also need to reorder the properties of the Offer struct, as outlined in Issue #45.

[sunsetlover]

Not sure if you saw my comment here: https://github.com/dapperlabs/cryptokitties-bounty-2/issues/39#issuecomment-439746889, but it is related. Initially, I thought the same as you. However, I am doubtful now that you can indeed save 5k gas by just updating 1 word in the 2 word struct due to the solidity compiler. I think the problem is--at least compiled with solidity-- you are rewriting both of the words in the struct even if one word stays the same (i.e. the code calls SSTORE twice) by setting it equal to an Offer type variable.

I just tried this with remix and it costs 10k gas to update according to your fix, so it does not save any gas.

The only fix I can think of to save gas is to create two mappings. One that contains the expiry time of an offer and one that contains the Offer struct except with the expiry time removed (so it is a struct with one 256 word). Then, you will only need to update the second mapping when someone updates their bid and it will only cost 5k gas even though you are changing two variables in the struct.

[ghost]

This issue still stands even if it's the case that we can only ever write the entire struct to storage.

The following lines are two separate writes to storage:

updatedOffer.total += uint128(msg.value);
updatedOffer.expiresAt = uint64(newExpiresAt);

It still would save gas to write to storage once rather than twice by using the temporary variable updatedOffer, as outlined above.

[hwrdtm]

Thanks @michaelKim4736 for the feedback! We will take this into consideration.

[sunsetlover]

Ah I see. Right. I guess another way to even save on top of that is two create two mappings as I’ve suggested. Then it would only cost 5k instead of the current 20k.

[arthcmr]

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

Severity: Note Points: 50