audit: Better Storage and Struct Packing Possible (#204)

* refactor: pack proposal struct into 4 slots * refactor: use uint96 for whitelist vp * chore: rerun snapshots --------- Co-authored-by: Orlando <[email protected]>
snapshot-labs · Jun 15, 2023 · c3c173f · c3c173f
1 parent ea3d686
commit c3c173f
Show file tree

Hide file tree

Showing 10 changed files with 42 additions and 34 deletions.
diff --git a/.forge-snapshots/ProposeSigComp.snap b/.forge-snapshots/ProposeSigComp.snap
@@ -1 +1 @@
-170947
+149973
diff --git a/.forge-snapshots/VoteSigComp.snap b/.forge-snapshots/VoteSigComp.snap
@@ -1 +1 @@
-50750
+50656
diff --git a/.forge-snapshots/VoteSigCompMetadata.snap b/.forge-snapshots/VoteSigCompMetadata.snap
@@ -1 +1 @@
-52301
+52207
diff --git a/.forge-snapshots/VoteTxComp.snap b/.forge-snapshots/VoteTxComp.snap
@@ -1 +1 @@
-44298
+44204
diff --git a/.forge-snapshots/VoteTxCompMetadata.snap b/.forge-snapshots/VoteTxCompMetadata.snap
@@ -1 +1 @@
-45889
+45795
diff --git a/src/Space.sol b/src/Space.sol
@@ -223,14 +223,14 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra
         bytes32 executionPayloadHash = keccak256(executionStrategy.params);
 
         Proposal memory proposal = Proposal(
+            author,
             snapshotTimestamp,
             startTimestamp,
+            IExecutionStrategy(executionStrategy.addr),
             minEndTimestamp,
             maxEndTimestamp,
-            executionPayloadHash,
-            IExecutionStrategy(executionStrategy.addr),
-            author,
             FinalizationStatus.Pending,
+            executionPayloadHash,
             activeVotingStrategies
         );
 

diff --git a/src/interfaces/space/ISpaceState.sol b/src/interfaces/space/ISpaceState.sol
@@ -54,30 +54,30 @@ interface ISpaceState {
     /// @notice Returns the proposal at a given ID.
     /// @dev Returns all zeros if the proposal does not exist.
     /// @param proposalId The ID of the proposal.
+    /// @return author The address of the proposal author.
     /// @return snapshotTimestamp The timestamp of the proposal snapshot.
     ///         All Voting Power will be calculated at this timestamp.
     /// @return startTimestamp The timestamp of the start of the voting period.
+    /// @return executionStrategy The address of the execution strategy used in the proposal.
     /// @return minEndTimestamp The timestamp of the minimum end of the voting period.
     /// @return maxEndTimestamp The timestamp of the maximum end of the voting period.
-    /// @return executionPayloadHash The keccak256 hash of the execution payload.
-    /// @return executionStrategy The address of the execution strategy used in the proposal.
-    /// @return author The address of the proposal author.
     /// @return finalizationStatus The finalization status of the proposal. See `FinalizationStatus`.
+    /// @return executionPayloadHash The keccak256 hash of the execution payload.
     /// @return activeVotingStrategies The bit array of the active voting strategies for the proposal.
     function proposals(
         uint256 proposalId
     )
         external
         view
         returns (
+            address author,
             uint32 snapshotTimestamp,
             uint32 startTimestamp,
+            IExecutionStrategy executionStrategy,
             uint32 minEndTimestamp,
             uint32 maxEndTimestamp,
-            bytes32 executionPayloadHash,
-            IExecutionStrategy executionStrategy,
-            address author,
             FinalizationStatus finalizationStatus,
+            bytes32 executionPayloadHash,
             uint256 activeVotingStrategies
         );
 

diff --git a/src/types.sol b/src/types.sol
@@ -6,23 +6,31 @@ import { Enum } from "@gnosis.pm/safe-contracts/contracts/common/Enum.sol";
 import { IExecutionStrategy } from "src/interfaces/IExecutionStrategy.sol";
 
 /// @notice The data stored for each proposal when it is created.
+/// @dev Packed into 4 256-bit slots.
 struct Proposal {
-    // The timestamp at which voting power for the proposal is calculated. Overflows at year ~2106.
+    // SLOT 1:
+    // The address of the proposal creator.
+    address author;
+    // The timestamp at which voting power for the proposal is calculated.
     uint32 snapshotTimestamp;
-    // We store the following 3 timestamps for each proposal despite the fact that they can be
-    // inferred from the votingDelay, minVotingDuration, and maxVotingDuration state variables
-    // because those variables may be updated during the lifetime of a proposal.
+    // The timestamp at which the voting period starts.
     uint32 startTimestamp;
-    uint32 minEndTimestamp;
-    uint32 maxEndTimestamp;
-    // The hash of the execution payload. We do not store the payload itself to save gas.
-    bytes32 executionPayloadHash;
+    //
+    // SLOT 2:
     // The address of execution strategy used for the proposal.
     IExecutionStrategy executionStrategy;
-    // The address of the proposal creator.
-    address author;
+    // The minimum timestamp at which the proposal can be finalized.
+    uint32 minEndTimestamp;
+    // The maximum timestamp at which the proposal can be finalized.
+    uint32 maxEndTimestamp;
     // An enum that stores whether a proposal is pending, executed, or cancelled.
     FinalizationStatus finalizationStatus;
+    //
+    // SLOT 3:
+    // The hash of the execution payload. We do not store the payload itself to save gas.
+    bytes32 executionPayloadHash;
+    //
+    // SLOT 4:
     // Bit array where the index of each each bit corresponds to whether the voting strategy.
     // at that index is active at the time of proposal creation.
     uint256 activeVotingStrategies;

diff --git a/src/voting-strategies/WhitelistVotingStrategy.sol b/src/voting-strategies/WhitelistVotingStrategy.sol
@@ -16,7 +16,7 @@ contract WhitelistVotingStrategy is IVotingStrategy {
         // The address of the member.
         address addr;
         // The voting power of the member.
-        uint256 vp;
+        uint96 vp;
     }
 
     /// @notice Returns the voting power of an address at a given timestamp.

diff --git a/test/Propose.t.sol b/test/Propose.t.sol
@@ -18,14 +18,14 @@ contract ProposeTest is SpaceTest {
 
         // Expected content of the proposal struct
         Proposal memory proposal = Proposal(
+            author,
             uint32(block.timestamp),
             uint32(block.timestamp + votingDelay),
+            IExecutionStrategy(executionStrategy.addr),
             uint32(block.timestamp + votingDelay + minVotingDuration),
             uint32(block.timestamp + votingDelay + maxVotingDuration),
-            keccak256(abi.encodePacked(executionStrategy.params)),
-            IExecutionStrategy(executionStrategy.addr),
-            author,
             FinalizationStatus.Pending,
+            keccak256(abi.encodePacked(executionStrategy.params)),
             activeVotingStrategies
         );
 
@@ -36,26 +36,26 @@ contract ProposeTest is SpaceTest {
 
         // Actual content of the proposal struct
         (
+            address _author,
             uint32 _snapshotTimestamp,
             uint32 _startTimestamp,
+            IExecutionStrategy _executionStrategy,
             uint32 _minEndTimestamp,
             uint32 _maxEndTimestamp,
-            bytes32 _executionPayloadHash,
-            IExecutionStrategy _executionStrategy,
-            address _author,
             FinalizationStatus _finalizationStatus,
+            bytes32 _executionPayloadHash,
             uint256 _activeVotingStrategies
         ) = space.proposals(proposalId);
 
         Proposal memory _proposal = Proposal(
+            _author,
             _snapshotTimestamp,
             _startTimestamp,
+            IExecutionStrategy(_executionStrategy),
             _minEndTimestamp,
             _maxEndTimestamp,
-            _executionPayloadHash,
-            IExecutionStrategy(_executionStrategy),
-            _author,
             _finalizationStatus,
+            _executionPayloadHash,
             _activeVotingStrategies
         );