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

audit: Better Storage and Struct Packing Possible #204

Merged
merged 4 commits into from
Jun 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .forge-snapshots/ProposeSigComp.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
170947
149973
2 changes: 1 addition & 1 deletion .forge-snapshots/VoteSigComp.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
50750
50656
2 changes: 1 addition & 1 deletion .forge-snapshots/VoteSigCompMetadata.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
52301
52207
2 changes: 1 addition & 1 deletion .forge-snapshots/VoteTxComp.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
44298
44204
2 changes: 1 addition & 1 deletion .forge-snapshots/VoteTxCompMetadata.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
45889
45795
6 changes: 3 additions & 3 deletions src/Space.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
);

Expand Down
12 changes: 6 additions & 6 deletions src/interfaces/space/ISpaceState.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
);

Expand Down
28 changes: 18 additions & 10 deletions src/types.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment on lines -12 to -14
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should keep that in the code as it's a useful comment for the reader imho

// 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;
pscott marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
2 changes: 1 addition & 1 deletion src/voting-strategies/WhitelistVotingStrategy.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this was mentioned in the audit but I don't know how I feel about it. We could imagine some people doing a token + whitelist strategy, and having like 2^100 tokens each. In which case, the whitelist would be too limited...

I guess this is an edge case scenario and 99% of the time, uint96 will do the work so it's fine

Copy link
Contributor Author

@Orland0x Orland0x Jun 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, always possible to just write a new strat too if you really need something specific

}

/// @notice Returns the voting power of an address at a given timestamp.
Expand Down
18 changes: 9 additions & 9 deletions test/Propose.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
);

Expand All @@ -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
);

Expand Down