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: use uint256 instead of bool in mappings #208

Merged
merged 9 commits into from
Jun 19, 2023
2 changes: 1 addition & 1 deletion .forge-snapshots/ProposeSigComp.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
148973
148841
2 changes: 1 addition & 1 deletion .forge-snapshots/VoteSigComp.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
50407
50322
2 changes: 1 addition & 1 deletion .forge-snapshots/VoteSigCompMetadata.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
51957
51865
2 changes: 1 addition & 1 deletion .forge-snapshots/VoteTxComp.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
43954
43892
2 changes: 1 addition & 1 deletion .forge-snapshots/VoteTxCompMetadata.snap
Original file line number Diff line number Diff line change
@@ -1 +1 @@
45545
45476
18 changes: 10 additions & 8 deletions src/Space.sol
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ import {
ProposalStatus,
Strategy,
UpdateSettingsCalldata,
InitializeCalldata
InitializeCalldata,
TRUE,
FALSE
} from "src/types.sol";
import { IVotingStrategy } from "src/interfaces/IVotingStrategy.sol";
import { IExecutionStrategy } from "src/interfaces/IExecutionStrategy.sol";
Expand Down Expand Up @@ -62,13 +64,13 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra
/// @inheritdoc ISpaceState
Strategy public override proposalValidationStrategy;
/// @inheritdoc ISpaceState
mapping(address auth => bool allowed) public override authenticators;
mapping(address auth => uint256 allowed) public override authenticators;
/// @inheritdoc ISpaceState
mapping(uint256 proposalId => Proposal proposal) public override proposals;
// @inheritdoc ISpaceState
mapping(uint256 proposalId => mapping(Choice choice => uint256 votePower)) public override votePower;
/// @inheritdoc ISpaceState
mapping(uint256 proposalId => mapping(address voter => bool hasVoted)) public override voteRegistry;
mapping(uint256 proposalId => mapping(address voter => uint256 hasVoted)) public override voteRegistry;

/// @inheritdoc ISpaceActions
function initialize(InitializeCalldata calldata input) external override initializer {
Expand Down Expand Up @@ -168,7 +170,7 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra

/// @dev Gates access to whitelisted authenticators only.
modifier onlyAuthenticator() {
if (authenticators[msg.sender] != true) revert AuthenticatorNotWhitelisted();
if (authenticators[msg.sender] != TRUE) revert AuthenticatorNotWhitelisted();
_;
}

Expand Down Expand Up @@ -250,9 +252,9 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra
if (block.number >= proposal.maxEndBlockNumber) revert VotingPeriodHasEnded();
if (block.number < proposal.startBlockNumber) revert VotingPeriodHasNotStarted();
if (proposal.finalizationStatus != FinalizationStatus.Pending) revert ProposalFinalized();
if (voteRegistry[proposalId][voter]) revert UserAlreadyVoted();
if (voteRegistry[proposalId][voter] == TRUE) revert UserAlreadyVoted();

voteRegistry[proposalId][voter] = true;
voteRegistry[proposalId][voter] = TRUE;

uint256 votingPower = _getCumulativePower(
voter,
Expand Down Expand Up @@ -384,14 +386,14 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra
/// @dev Adds an array of authenticators.
function _addAuthenticators(address[] calldata _authenticators) internal {
for (uint256 i = 0; i < _authenticators.length; i++) {
authenticators[_authenticators[i]] = true;
authenticators[_authenticators[i]] = TRUE;
}
}

/// @dev Removes an array of authenticators.
function _removeAuthenticators(address[] calldata _authenticators) internal {
for (uint256 i = 0; i < _authenticators.length; i++) {
authenticators[_authenticators[i]] = false;
authenticators[_authenticators[i]] = FALSE;
}
// TODO: should we check that there are still authenticators left? same for other setters..
}
Expand Down
4 changes: 2 additions & 2 deletions src/interfaces/space/ISpaceState.sol
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ interface ISpaceState {
function votingDelay() external view returns (uint32);

/// @notice Returns whether a given address is a whitelisted authenticator.
function authenticators(address) external view returns (bool);
function authenticators(address) external view returns (uint256);

/// @notice Returns the voting strategy at a given index.
/// @param index The index of the voting strategy.
Expand Down Expand Up @@ -49,7 +49,7 @@ interface ISpaceState {
/// @notice Returns whether a voter has voted on a proposal.
/// @param proposalId The ID of the proposal.
/// @param voter The address of the voter.
function voteRegistry(uint256 proposalId, address voter) external view returns (bool);
function voteRegistry(uint256 proposalId, address voter) external view returns (uint256);

/// @notice Returns the proposal at a given ID.
/// @dev Returns all zeros if the proposal does not exist.
Expand Down
4 changes: 4 additions & 0 deletions src/types.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ pragma solidity ^0.8.18;
import { Enum } from "@gnosis.pm/safe-contracts/contracts/common/Enum.sol";
import { IExecutionStrategy } from "src/interfaces/IExecutionStrategy.sol";

/// @dev Constants used to replace the `bool` type in mappings for gas efficiency.
uint256 constant TRUE = 1;
uint256 constant FALSE = 0;

/// @notice The data stored for each proposal when it is created.
/// @dev Packed into 4 256-bit slots.
struct Proposal {
Expand Down
11 changes: 6 additions & 5 deletions src/utils/SignatureVerifier.sol
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { ECDSA } from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol";
import { EIP712 } from "@openzeppelin/contracts/utils/cryptography/EIP712.sol";
import { Choice, IndexedStrategy, Strategy } from "src/types.sol";
import { SXHash } from "src/utils/SXHash.sol";
import { TRUE, FALSE } from "../types.sol";

/// @title EIP712 Signature Verifier
/// @notice Verifies Signatures for Snapshot X actions.
Expand Down Expand Up @@ -39,7 +40,7 @@ abstract contract SignatureVerifier is EIP712 {
"Strategy(address addr,bytes params)"
);

mapping(address author => mapping(uint256 salt => bool used)) private usedSalts;
mapping(address author => mapping(uint256 salt => uint256 used)) private usedSalts;

// solhint-disable-next-line no-empty-blocks
constructor(string memory name, string memory version) EIP712(name, version) {}
Expand All @@ -54,7 +55,7 @@ abstract contract SignatureVerifier is EIP712 {
bytes memory userProposalValidationParams
) = abi.decode(data, (address, string, Strategy, bytes));

if (usedSalts[author][salt]) revert SaltAlreadyUsed();
if (usedSalts[author][salt] == TRUE) revert SaltAlreadyUsed();

address recoveredAddress = ECDSA.recover(
_hashTypedDataV4(
Expand All @@ -78,7 +79,7 @@ abstract contract SignatureVerifier is EIP712 {
if (recoveredAddress != author) revert InvalidSignature();

// Mark salt as used to prevent replay attacks.
usedSalts[author][salt] = true;
usedSalts[author][salt] = TRUE;
}

/// @dev Verifies an EIP712 signature for a vote call.
Expand Down Expand Up @@ -128,7 +129,7 @@ abstract contract SignatureVerifier is EIP712 {
(address, uint256, Strategy, string)
);

if (usedSalts[author][salt]) revert SaltAlreadyUsed();
if (usedSalts[author][salt] == TRUE) revert SaltAlreadyUsed();

address recoveredAddress = ECDSA.recover(
_hashTypedDataV4(
Expand All @@ -152,6 +153,6 @@ abstract contract SignatureVerifier is EIP712 {
if (recoveredAddress != author) revert InvalidSignature();

// Mark salt as used to prevent replay attacks.
usedSalts[author][salt] = true;
usedSalts[author][salt] = TRUE;
}
}
19 changes: 10 additions & 9 deletions src/utils/SpaceManager.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,15 @@
pragma solidity ^0.8.18;

import { OwnableUpgradeable } from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol";
import { TRUE, FALSE } from "../types.sol";

/// @title Space Manager
/// @notice Manages a whitelist of Spaces that are authorized to execute transactions via this contract.
contract SpaceManager is OwnableUpgradeable {
/// @notice Thrown if a space is not in the whitelist.
error InvalidSpace();

mapping(address space => bool isEnabled) internal spaces;
mapping(address space => uint256 isEnabled) internal spaces;

/// @notice Emitted when a space is enabled.
event SpaceEnabled(address space);
Expand All @@ -23,35 +24,35 @@ contract SpaceManager is OwnableUpgradeable {
// solhint-disable-next-line func-name-mixedcase
function __SpaceManager_init(address[] memory _spaces) internal onlyInitializing {
for (uint256 i = 0; i < _spaces.length; i++) {
spaces[_spaces[i]] = true;
spaces[_spaces[i]] = TRUE;
}
}

/// @notice Enable a space.
/// @param space Address of the space.
function enableSpace(address space) external onlyOwner {
if (space == address(0) || spaces[space]) revert InvalidSpace();
spaces[space] = true;
if (space == address(0) || (spaces[space] == TRUE)) revert InvalidSpace();
spaces[space] = TRUE;
emit SpaceEnabled(space);
}

/// @notice Disable a space.
/// @param space Address of the space.
function disableSpace(address space) external onlyOwner {
if (!spaces[space]) revert InvalidSpace();
spaces[space] = false;
if (spaces[space] == FALSE) revert InvalidSpace();
spaces[space] = FALSE;
emit SpaceDisabled(space);
}

/// @notice Check if a space is enabled.
/// @param space Address of the space.
/// @return bool whether the space is enabled.
function isSpaceEnabled(address space) external view returns (bool) {
/// @return uint256 whether the space is enabled.
function isSpaceEnabled(address space) external view returns (uint256) {
return spaces[space];
}

modifier onlySpace() {
if (!spaces[msg.sender]) revert InvalidSpace();
if (spaces[msg.sender] == FALSE) revert InvalidSpace();
_;
}
}
15 changes: 12 additions & 3 deletions test/AvatarExecutionStrategy.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,16 @@ pragma solidity ^0.8.18;
import { SpaceTest } from "./utils/Space.t.sol";
import { Avatar } from "./mocks/Avatar.sol";
import { AvatarExecutionStrategy } from "../src/execution-strategies/AvatarExecutionStrategy.sol";
import { Choice, Enum, IndexedStrategy, MetaTransaction, ProposalStatus, Strategy } from "../src/types.sol";
import {
Choice,
Enum,
IndexedStrategy,
MetaTransaction,
ProposalStatus,
Strategy,
TRUE,
FALSE
} from "../src/types.sol";
import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";

abstract contract AvatarExecutionStrategyTest is SpaceTest {
Expand Down Expand Up @@ -196,7 +205,7 @@ abstract contract AvatarExecutionStrategyTest is SpaceTest {
vm.expectEmit(true, true, true, true);
emit SpaceEnabled(space);
avatarExecutionStrategy.enableSpace(space);
assertEq(avatarExecutionStrategy.isSpaceEnabled(space), true);
assertEq(avatarExecutionStrategy.isSpaceEnabled(space), TRUE);
}

function testEnableInvalidSpace() public {
Expand Down Expand Up @@ -225,7 +234,7 @@ abstract contract AvatarExecutionStrategyTest is SpaceTest {
vm.expectEmit(true, true, true, true);
emit SpaceDisabled(address(space));
avatarExecutionStrategy.disableSpace(address(space));
assertEq(avatarExecutionStrategy.isSpaceEnabled(address(space)), false);
assertEq(avatarExecutionStrategy.isSpaceEnabled(address(space)), FALSE);

// Check that proposals from the disabled space can't be executed
MetaTransaction[] memory transactions = new MetaTransaction[](1);
Expand Down
14 changes: 12 additions & 2 deletions test/CompTimelockExecutionStrategy.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,17 @@
pragma solidity ^0.8.18;

import { SpaceTest } from "./utils/Space.t.sol";
import { Choice, Enum, IndexedStrategy, MetaTransaction, ProposalStatus, Strategy, Proposal } from "../src/types.sol";
import {
Choice,
Enum,
IndexedStrategy,
MetaTransaction,
ProposalStatus,
Strategy,
Proposal,
TRUE,
FALSE
} from "../src/types.sol";
import {
CompTimelockCompatibleExecutionStrategy
} from "../src/execution-strategies/CompTimelockCompatibleExecutionStrategy.sol";
Expand Down Expand Up @@ -480,7 +490,7 @@ abstract contract CompTimelockExecutionStrategyTest is SpaceTest {
assertEq(timelockExecutionStrategy.vetoGuardian(), vetoGuardian);
assertEq(timelockExecutionStrategy.quorum(), quorum);
assertEq(address(timelockExecutionStrategy.timelock()), address(timelock));
assertEq(timelockExecutionStrategy.isSpaceEnabled(address(space)), true);
assertEq(timelockExecutionStrategy.isSpaceEnabled(address(space)), TRUE);
}
}

Expand Down
14 changes: 7 additions & 7 deletions test/SpaceOwnerActions.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
pragma solidity ^0.8.18;

import { SpaceV2 } from "./mocks/SpaceV2.sol";
import { SpaceTest } from "./utils/Space.t.sol";
import { TRUE, FALSE, SpaceTest } from "./utils/Space.t.sol";
import { Choice, IndexedStrategy, Strategy, UpdateSettingsCalldata } from "../src/types.sol";
import { VanillaExecutionStrategy } from "../src/execution-strategies/VanillaExecutionStrategy.sol";
import { BitPacker } from "../src/utils/BitPacker.sol";
Expand Down Expand Up @@ -590,9 +590,9 @@ contract SpaceOwnerActionsTest is SpaceTest {
);

// Ensure authenticators were correctly updated
assertEq(space.authenticators(newAuths[0]), true);
assertEq(space.authenticators(newAuths[1]), true);
assertEq(space.authenticators(authenticators[0]), false);
assertEq(space.authenticators(newAuths[0]), TRUE);
assertEq(space.authenticators(newAuths[1]), TRUE);
assertEq(space.authenticators(authenticators[0]), FALSE);
}

function testUpdateVotingStrategies() public {
Expand Down Expand Up @@ -701,9 +701,9 @@ contract SpaceOwnerActionsTest is SpaceTest {
assertEq(params, _proposalValidationStrategy.params);

// Ensure authenticators were correctly updated
assertEq(space.authenticators(newAuths[0]), true);
assertEq(space.authenticators(newAuths[1]), true);
assertEq(space.authenticators(authenticators[0]), false);
assertEq(space.authenticators(newAuths[0]), TRUE);
assertEq(space.authenticators(newAuths[1]), TRUE);
assertEq(space.authenticators(authenticators[0]), FALSE);

// Ensure voting strategies were correctly updated
assertEq(space.activeVotingStrategies().isBitSet(0), false);
Expand Down
14 changes: 12 additions & 2 deletions test/TimelockExecutionStrategy.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,17 @@
pragma solidity ^0.8.18;

import { SpaceTest } from "./utils/Space.t.sol";
import { Choice, Enum, IndexedStrategy, MetaTransaction, ProposalStatus, Strategy, Proposal } from "../src/types.sol";
import {
Choice,
Enum,
IndexedStrategy,
MetaTransaction,
ProposalStatus,
Strategy,
Proposal,
TRUE,
FALSE
} from "../src/types.sol";
import { TimelockExecutionStrategy } from "../src/execution-strategies/TimelockExecutionStrategy.sol";
import { MockImplementation } from "./mocks/MockImplementation.sol";
import { ERC1967Proxy } from "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
Expand Down Expand Up @@ -490,7 +500,7 @@ abstract contract TimelockExecutionStrategyTest is SpaceTest {
assertEq(timelockExecutionStrategy.vetoGuardian(), vetoGuardian);
assertEq(timelockExecutionStrategy.quorum(), quorum);
assertEq(timelockExecutionStrategy.timelockDelay(), 1000);
assertEq(timelockExecutionStrategy.isSpaceEnabled(address(space)), true);
assertEq(timelockExecutionStrategy.isSpaceEnabled(address(space)), TRUE);
}
}

Expand Down
4 changes: 2 additions & 2 deletions test/Vote.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

pragma solidity ^0.8.18;

import { SpaceTest } from "./utils/Space.t.sol";
import { TRUE, FALSE, SpaceTest } from "./utils/Space.t.sol";
import { Choice, IndexedStrategy, Strategy, UpdateSettingsCalldata } from "../src/types.sol";
import { VanillaVotingStrategy } from "../src/voting-strategies/VanillaVotingStrategy.sol";

Expand All @@ -20,7 +20,7 @@ contract VoteTest is SpaceTest {
abi.encode(author, proposalId, Choice.For, userVotingStrategies, voteMetadataURI)
);

assertTrue(space.voteRegistry(proposalId, author));
assertEq(space.voteRegistry(proposalId, author), TRUE);
}

function testVoteInvalidAuth() public {
Expand Down
2 changes: 1 addition & 1 deletion test/utils/Space.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import {
import { ISpaceEvents } from "../../src/interfaces/space/ISpaceEvents.sol";
import { ISpaceErrors } from "../../src/interfaces/space/ISpaceErrors.sol";
import { IExecutionStrategyErrors } from "../../src/interfaces/execution-strategies/IExecutionStrategyErrors.sol";
import { Choice, Strategy, IndexedStrategy, InitializeCalldata } from "../../src/types.sol";
import { Choice, Strategy, IndexedStrategy, InitializeCalldata, TRUE, FALSE } from "../../src/types.sol";

// solhint-disable-next-line max-states-count
abstract contract SpaceTest is Test, GasSnapshot, ISpaceEvents, ISpaceErrors, IExecutionStrategyErrors {
Expand Down