diff --git a/src/ProxyFactory.sol b/src/ProxyFactory.sol index e957d538..674746be 100644 --- a/src/ProxyFactory.sol +++ b/src/ProxyFactory.sol @@ -26,7 +26,7 @@ contract ProxyFactory is IProxyFactory { return address( uint160( - uint( + uint256( keccak256( abi.encodePacked( bytes1(0xff), diff --git a/src/Space.sol b/src/Space.sol index 1653ad38..8338306e 100644 --- a/src/Space.sol +++ b/src/Space.sol @@ -107,8 +107,9 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra // We don't use the internal `_setMinVotingDuration` and `_setMaxVotingDuration` functions because // it would revert when `_minVotingDuration > maxVotingDuration` (when the new `_min` is // bigger than the current `max`). - if (input.minVotingDuration > input.maxVotingDuration) + if (input.minVotingDuration > input.maxVotingDuration) { revert InvalidDuration(input.minVotingDuration, input.maxVotingDuration); + } minVotingDuration = input.minVotingDuration; emit MinVotingDurationUpdated(input.minVotingDuration); @@ -156,8 +157,9 @@ contract Space is ISpace, Initializable, IERC4824, UUPSUpgradeable, OwnableUpgra } if (input.votingStrategiesToAdd.length > 0) { - if (input.votingStrategiesToAdd.length != input.votingStrategyMetadataURIsToAdd.length) + if (input.votingStrategiesToAdd.length != input.votingStrategyMetadataURIsToAdd.length) { revert ArrayLengthMismatch(); + } _addVotingStrategies(input.votingStrategiesToAdd); emit VotingStrategiesAdded(input.votingStrategiesToAdd, input.votingStrategyMetadataURIsToAdd); } diff --git a/src/interfaces/ICompTimelock.sol b/src/interfaces/ICompTimelock.sol index 94c01ee8..14ccd229 100644 --- a/src/interfaces/ICompTimelock.sol +++ b/src/interfaces/ICompTimelock.sol @@ -15,10 +15,10 @@ interface ICompTimelock { /// @return The transaction hash. function queueTransaction( address target, - uint value, + uint256 value, string memory signature, bytes memory data, - uint eta + uint256 eta ) external returns (bytes32); /// @notice Execute a queued transaction. @@ -30,10 +30,10 @@ interface ICompTimelock { /// @return The transaction return data. function executeTransaction( address target, - uint value, + uint256 value, string memory signature, bytes memory data, - uint eta + uint256 eta ) external payable returns (bytes memory); /// @notice Cancel a queued transaction. @@ -44,21 +44,21 @@ interface ICompTimelock { /// @param eta The timestamp at which to execute the transaction, in seconds. function cancelTransaction( address target, - uint value, + uint256 value, string memory signature, bytes memory data, - uint eta + uint256 eta ) external; - function setDelay(uint delay) external; + function setDelay(uint256 delay) external; - function GRACE_PERIOD() external view returns (uint); + function GRACE_PERIOD() external view returns (uint256); - function MINIMUM_DELAY() external view returns (uint); + function MINIMUM_DELAY() external view returns (uint256); - function MAXIMUM_DELAY() external view returns (uint); + function MAXIMUM_DELAY() external view returns (uint256); - function delay() external view returns (uint); + function delay() external view returns (uint256); function queuedTransactions(bytes32 hash) external view returns (bool); } diff --git a/src/utils/SignatureVerifier.sol b/src/utils/SignatureVerifier.sol index 7e6619d4..d93b2e2a 100644 --- a/src/utils/SignatureVerifier.sol +++ b/src/utils/SignatureVerifier.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.18; import { ECDSA } from "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import { EIP712 } from "@openzeppelin/contracts/utils/cryptography/EIP712.sol"; +import { SignatureChecker } from "@openzeppelin/contracts/utils/cryptography/SignatureChecker.sol"; import { Choice, IndexedStrategy, Strategy } from "src/types.sol"; import { SXHash } from "src/utils/SXHash.sol"; import { TRUE, FALSE } from "../types.sol"; @@ -56,30 +57,28 @@ abstract contract SignatureVerifier is EIP712 { ) = abi.decode(data, (address, string, Strategy, bytes)); if (usedSalts[author][salt] != FALSE) revert SaltAlreadyUsed(); + // Mark salt as used to prevent replay attacks. + usedSalts[author][salt] = TRUE; - address recoveredAddress = ECDSA.recover( - _hashTypedDataV4( - keccak256( - abi.encode( - PROPOSE_TYPEHASH, - space, - author, - keccak256(bytes(metadataURI)), - executionStrategy.hash(), - keccak256(userProposalValidationParams), - salt - ) + bytes32 messageHash = _hashTypedDataV4( + keccak256( + abi.encode( + PROPOSE_TYPEHASH, + space, + author, + keccak256(bytes(metadataURI)), + executionStrategy.hash(), + keccak256(userProposalValidationParams), + salt ) - ), - v, - r, - s + ) ); - if (recoveredAddress != author) revert InvalidSignature(); + bytes memory signature = abi.encodePacked(r, s, v); - // Mark salt as used to prevent replay attacks. - usedSalts[author][salt] = TRUE; + bool valid = SignatureChecker.isValidSignatureNow(author, messageHash, signature); + + if (!valid) revert InvalidSignature(); } /// @dev Verifies an EIP712 signature for a vote call. @@ -92,26 +91,25 @@ abstract contract SignatureVerifier is EIP712 { string memory voteMetadataURI ) = abi.decode(data, (address, uint256, Choice, IndexedStrategy[], string)); - address recoveredAddress = ECDSA.recover( - _hashTypedDataV4( - keccak256( - abi.encode( - VOTE_TYPEHASH, - space, - voter, - proposeId, - choice, - userVotingStrategies.hash(), - keccak256(bytes(voteMetadataURI)) - ) + bytes32 messageHash = _hashTypedDataV4( + keccak256( + abi.encode( + VOTE_TYPEHASH, + space, + voter, + proposeId, + choice, + userVotingStrategies.hash(), + keccak256(bytes(voteMetadataURI)) ) - ), - v, - r, - s + ) ); - if (recoveredAddress != voter) revert InvalidSignature(); + bytes memory signature = abi.encodePacked(r, s, v); + + bool valid = SignatureChecker.isValidSignatureNow(voter, messageHash, signature); + + if (!valid) revert InvalidSignature(); } /// @dev Verifies an EIP712 signature for an update proposal call. @@ -130,29 +128,27 @@ abstract contract SignatureVerifier is EIP712 { ); if (usedSalts[author][salt] != FALSE) revert SaltAlreadyUsed(); + // Mark salt as used to prevent replay attacks. + usedSalts[author][salt] = TRUE; - address recoveredAddress = ECDSA.recover( - _hashTypedDataV4( - keccak256( - abi.encode( - UPDATE_PROPOSAL_TYPEHASH, - space, - author, - proposalId, - executionStrategy.hash(), - keccak256(bytes(metadataURI)), - salt - ) + bytes32 messageHash = _hashTypedDataV4( + keccak256( + abi.encode( + UPDATE_PROPOSAL_TYPEHASH, + space, + author, + proposalId, + executionStrategy.hash(), + keccak256(bytes(metadataURI)), + salt ) - ), - v, - r, - s + ) ); - if (recoveredAddress != author) revert InvalidSignature(); + bytes memory signature = abi.encodePacked(r, s, v); - // Mark salt as used to prevent replay attacks. - usedSalts[author][salt] = TRUE; + bool valid = SignatureChecker.isValidSignatureNow(author, messageHash, signature); + + if (!valid) revert InvalidSignature(); } } diff --git a/src/voting-strategies/MerkleWhitelistVotingStrategy.sol b/src/voting-strategies/MerkleWhitelistVotingStrategy.sol index 2c1bec6d..1847eb77 100644 --- a/src/voting-strategies/MerkleWhitelistVotingStrategy.sol +++ b/src/voting-strategies/MerkleWhitelistVotingStrategy.sol @@ -28,7 +28,7 @@ contract MerkleWhitelistVotingStrategy is IVotingStrategy { /// @param userParams Parameter array containing the desired member of the whitelist and its associated merkle proof. /// @return votingPower The voting power of the address if it exists in the whitelist, otherwise reverts. function getVotingPower( - uint32 /* blockNumber */, + uint32 _blockNumber, address voter, bytes calldata params, bytes calldata userParams diff --git a/src/voting-strategies/WhitelistVotingStrategy.sol b/src/voting-strategies/WhitelistVotingStrategy.sol index 4047936e..a5d65082 100644 --- a/src/voting-strategies/WhitelistVotingStrategy.sol +++ b/src/voting-strategies/WhitelistVotingStrategy.sol @@ -26,7 +26,7 @@ contract WhitelistVotingStrategy is IVotingStrategy { /// @param userParams Expected to contain a `uint256` corresponding to the voterIndex in the array provided by `params`. /// @return votingPower The voting power of the address if it exists in the whitelist, otherwise reverts. function getVotingPower( - uint32 /* blockNumber */, + uint32 _blockNumber, address voter, bytes calldata params, bytes calldata userParams diff --git a/test/CompTimelockExecutionStrategy.t.sol b/test/CompTimelockExecutionStrategy.t.sol index 06cdd299..4120396c 100644 --- a/test/CompTimelockExecutionStrategy.t.sol +++ b/test/CompTimelockExecutionStrategy.t.sol @@ -31,6 +31,7 @@ abstract contract CompTimelockExecutionStrategyTest is SpaceTest { error DuplicateMetaTransaction(); error OnlyVetoGuardian(); error InvalidTransaction(); + event CompTimelockCompatibleExecutionStrategySetUp( address owner, address vetoGuardian, diff --git a/test/OptimisticCompTimelockExecutionStrategy.t.sol b/test/OptimisticCompTimelockExecutionStrategy.t.sol index 15393ba1..6603eaa8 100644 --- a/test/OptimisticCompTimelockExecutionStrategy.t.sol +++ b/test/OptimisticCompTimelockExecutionStrategy.t.sol @@ -32,6 +32,7 @@ abstract contract OptimisticCompTimelockExecutionStrategyTest is SpaceTest { error DuplicateMetaTransaction(); error OnlyVetoGuardian(); error InvalidTransaction(); + event OptimisticCompTimelockCompatibleExecutionStrategySetUp( address owner, address vetoGuardian, diff --git a/test/OptimisticTimelockExecutionStrategy.t.sol b/test/OptimisticTimelockExecutionStrategy.t.sol index 52caf411..af35ed75 100644 --- a/test/OptimisticTimelockExecutionStrategy.t.sol +++ b/test/OptimisticTimelockExecutionStrategy.t.sol @@ -32,6 +32,7 @@ abstract contract OptimisticTimelockExecutionStrategyTest is SpaceTest { error ProposalNotQueued(); error DuplicateExecutionPayloadHash(); error OnlyVetoGuardian(); + event OptimisticTimelockExecutionStrategySetUp( address owner, address vetoGuardian, diff --git a/test/ProxyFactory.t.sol b/test/ProxyFactory.t.sol index 14e8f88d..0a28592e 100644 --- a/test/ProxyFactory.t.sol +++ b/test/ProxyFactory.t.sol @@ -243,7 +243,7 @@ contract SpaceFactoryTest is Test, IProxyFactoryEvents, IProxyFactoryErrors, ISp return address( uint160( - uint( + uint256( keccak256( abi.encodePacked( bytes1(0xff), diff --git a/test/TimelockExecutionStrategy.t.sol b/test/TimelockExecutionStrategy.t.sol index 8b4aafca..dabd664f 100644 --- a/test/TimelockExecutionStrategy.t.sol +++ b/test/TimelockExecutionStrategy.t.sol @@ -29,6 +29,7 @@ abstract contract TimelockExecutionStrategyTest is SpaceTest { error ProposalNotQueued(); error DuplicateExecutionPayloadHash(); error OnlyVetoGuardian(); + event TimelockExecutionStrategySetUp( address owner, address vetoGuardian, diff --git a/test/VotingPowerProposalValidationStrategy.t.sol b/test/VotingPowerProposalValidationStrategy.t.sol index e5a4cba5..c3108caa 100644 --- a/test/VotingPowerProposalValidationStrategy.t.sol +++ b/test/VotingPowerProposalValidationStrategy.t.sol @@ -12,6 +12,7 @@ import { CompToken } from "./mocks/CompToken.sol"; contract PropositionPowerProposalValidationTest is SpaceTest { error DuplicateFound(uint8 index); + uint256 internal proposalThreshold = 100; CompToken internal compToken; IndexedStrategy[] internal userPropositionPowerStrategies; diff --git a/test/mocks/CompTimelock.sol b/test/mocks/CompTimelock.sol index 561f5455..f9cc8c1e 100644 --- a/test/mocks/CompTimelock.sol +++ b/test/mocks/CompTimelock.sol @@ -4,43 +4,43 @@ pragma solidity ^0.8.18; contract CompTimelock { event NewAdmin(address indexed newAdmin); event NewPendingAdmin(address indexed newPendingAdmin); - event NewDelay(uint indexed newDelay); + event NewDelay(uint256 indexed newDelay); event CancelTransaction( bytes32 indexed txHash, address indexed target, - uint value, + uint256 value, string signature, bytes data, - uint eta + uint256 eta ); event ExecuteTransaction( bytes32 indexed txHash, address indexed target, - uint value, + uint256 value, string signature, bytes data, - uint eta + uint256 eta ); event QueueTransaction( bytes32 indexed txHash, address indexed target, - uint value, + uint256 value, string signature, bytes data, - uint eta + uint256 eta ); - uint public constant GRACE_PERIOD = 14 days; - uint public constant MINIMUM_DELAY = 10; - uint public constant MAXIMUM_DELAY = 30 days; + uint256 public constant GRACE_PERIOD = 14 days; + uint256 public constant MINIMUM_DELAY = 10; + uint256 public constant MAXIMUM_DELAY = 30 days; address public admin; address public pendingAdmin; - uint public delay; + uint256 public delay; mapping(bytes32 => bool) public queuedTransactions; - constructor(address admin_, uint delay_) public { + constructor(address admin_, uint256 delay_) public { require(delay_ >= MINIMUM_DELAY, "Timelock::constructor: Delay must exceed minimum delay."); require(delay_ <= MAXIMUM_DELAY, "Timelock::setDelay: Delay must not exceed maximum delay."); @@ -50,7 +50,7 @@ contract CompTimelock { fallback() external payable {} - function setDelay(uint delay_) public { + function setDelay(uint256 delay_) public { require(msg.sender == address(this), "Timelock::setDelay: Call must come from Timelock."); require(delay_ >= MINIMUM_DELAY, "Timelock::setDelay: Delay must exceed minimum delay."); require(delay_ <= MAXIMUM_DELAY, "Timelock::setDelay: Delay must not exceed maximum delay."); @@ -76,10 +76,10 @@ contract CompTimelock { function queueTransaction( address target, - uint value, + uint256 value, string memory signature, bytes memory data, - uint eta + uint256 eta ) public returns (bytes32) { require(msg.sender == admin, "Timelock::queueTransaction: Call must come from admin."); require( @@ -96,10 +96,10 @@ contract CompTimelock { function cancelTransaction( address target, - uint value, + uint256 value, string memory signature, bytes memory data, - uint eta + uint256 eta ) public { require(msg.sender == admin, "Timelock::cancelTransaction: Call must come from admin."); @@ -111,10 +111,10 @@ contract CompTimelock { function executeTransaction( address target, - uint value, + uint256 value, string memory signature, bytes memory data, - uint eta + uint256 eta ) public payable returns (bytes memory) { require(msg.sender == admin, "Timelock::executeTransaction: Call must come from admin."); @@ -142,7 +142,7 @@ contract CompTimelock { return returnData; } - function getBlockTimestamp() internal view returns (uint) { + function getBlockTimestamp() internal view returns (uint256) { // solium-disable-next-line security/no-block-members return block.timestamp; } diff --git a/test/utils/Authenticator.t.sol b/test/utils/Authenticator.t.sol index c97d8dab..cd145b28 100644 --- a/test/utils/Authenticator.t.sol +++ b/test/utils/Authenticator.t.sol @@ -6,6 +6,7 @@ import { Test } from "forge-std/Test.sol"; contract DemoTarget { uint256 public x; + error TooBig(); function foo(uint256 _x) public {