Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tight Variable Packing #32

Open
code423n4 opened this issue Aug 24, 2021 · 3 comments
Open

Tight Variable Packing #32

code423n4 opened this issue Aug 24, 2021 · 3 comments

Comments

@code423n4
Copy link
Contributor

Handle

leastwood

Vulnerability details

Impact

While there is no reward for gas optimizations, it could still prove useful to suggest relatively easy to implement optimizations with decent gas savings. Solidity contracts have contiguous 32 byte (256 bit) slots used in storage. By arranging the variables, it is possible to minimize the number of slots used within a contract's storage and therefore reduce deployment costs. The contract variables in RCMarket.sol and RCFactory.sol can be optimized in such a way. i.e. the bools can be packed alongside the enums in RCMarket.sol. Similarly, the uint32 types can be packed with the bools in RCFactory.sol.

Proof of Concept

https://github.com/code-423n4/2021-08-realitycards/blob/main/contracts/RCMarket.sol#L24-L132
https://github.com/code-423n4/2021-08-realitycards/blob/main/contracts/RCFactory.sol#L23-L89

Tools Used

Manual code review

Recommended Mitigation Steps

Pack the variables in RCFactory.sol and RCMarket.sol, such that the number of slots used in deployment is minimised.

@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels Aug 24, 2021
code423n4 added a commit that referenced this issue Aug 24, 2021
@Splidge
Copy link
Collaborator

Splidge commented Aug 26, 2021

While there could be some benefits had here I think it's less than it seems at first glance.
In the Market the artist, affiliate and cardAffiliate variables could be packed into less storage (reducing deployment costs) but at the expense at increased gas costs because when reading or writing to a storage slot the entire slot must be read even if only a portion of it is required.
The marketOpeningTime, marketLockingTime and oracleResolutionTime are all packed together already.
There's not many more variables that don't take up an entire memory slot to themselves.

It's a similar story with the factory, maybe timeout and marketPausedDefaultState could be packed in tighter but the others aren't too bad. advancedWarning, maximumDuration and minimumDuration are perfect being together and all being set at the same time.

I'm unsure how to classify this issue (disputed vs acknowledged), I leaning towards disputed because:

  • there's very little (if any) optimization to be had here.
  • it's uncertain and difficult to quantify if the extra cost of deployment wouldn't be outweighed by the gas savings of not having to read/write tightly packed variables.
  • packing storage is a common solidity dev task, without an example of where an improvement can be made this is simply throwing around 'good practice' just to see if it sticks.

@0xean
Copy link
Collaborator

0xean commented Sep 2, 2021

Warden's suggestion is valid but could certainly use some more detail. Leaving as a valid optimization none the less.

@Splidge
Copy link
Collaborator

Splidge commented Sep 7, 2021

Changing to acknowledged based on judges feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants