Skip to content

Commit

Permalink
mitigations: #306 (QA-2, QA-3, QA-5) (#354)
Browse files Browse the repository at this point in the history 
* implement QA-2, QA-3, and QA-5 suggestions

* fix `voteDuration` check

* fix tests
  • Loading branch information
@0xble
0xble authored Dec 4, 2023
1 parent d155f83 commit 65334de
Show file tree
Hide file tree
Showing 13 changed files with 38 additions and 30 deletions.
4 changes: 4 additions & 0 deletions contracts/crowdfund/ETHCrowdfundBase.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ abstract contract ETHCrowdfundBase is Implementation {
error BelowMinimumContributionsError(uint96 contributions, uint96 minContributions);
error AboveMaximumContributionsError(uint96 contributions, uint96 maxContributions);
error InvalidExchangeRateError(uint16 exchangeRateBps);
error InvalidFundingSplitRecipient();
error ContributingForExistingCardDisabledError();
error ZeroVotingPowerError();
error FundingSplitAlreadyPaidError();
Expand Down Expand Up @@ -167,6 +168,9 @@ abstract contract ETHCrowdfundBase is Implementation {
// Set the funding split and its recipient.
fundingSplitBps = opts.fundingSplitBps;
fundingSplitRecipient = opts.fundingSplitRecipient;
if (opts.fundingSplitBps > 0 && opts.fundingSplitRecipient == address(0)) {
revert InvalidFundingSplitRecipient();
}
// Set whether to disable contributing for existing card.
disableContributingForExistingCard = opts.disableContributingForExistingCard;
}
Expand Down
9 changes: 4 additions & 5 deletions contracts/crowdfund/InitialETHCrowdfund.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,10 +156,9 @@ contract InitialETHCrowdfund is ETHCrowdfundBase {
gateKeeperId = crowdfundOpts.gateKeeperId;
}

/// @notice Contribute ETH to this crowdfund on behalf of a contributor.
/// @param initialDelegate The address to which voting power will be delegated to
/// during the governance phase. This will be ignored
/// if recipient has already set a delegate.
/// @notice Contribute ETH to this crowdfund.
/// @param initialDelegate The address to which voting power will be
/// delegated to during the governance phase.
/// @param gateData Data to pass to the gatekeeper to prove eligibility.
/// @return votingPower The voting power the contributor receives for their
/// contribution.
Expand All @@ -177,7 +176,7 @@ contract InitialETHCrowdfund is ETHCrowdfundBase {
);
}

/// @notice Contribute ETH to this crowdfund on behalf of a contributor.
/// @notice Contribute ETH to this crowdfund.
/// @param tokenId The ID of the card the contribution is being made towards.
/// @param initialDelegate The address to which voting power will be delegated to
/// during the governance phase. This will be ignored
Expand Down
4 changes: 4 additions & 0 deletions contracts/party/PartyGovernance.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@ abstract contract PartyGovernance is
error InvalidNewHostError();
error ProposalCannotBeCancelledYetError(uint40 currentTime, uint40 cancelTime);
error InvalidBpsError(uint16 bps);
error InvalidGovernanceParameter(uint256 value);
error DistributionsRequireVoteError();
error PartyNotStartedError();
error CannotRageQuitAndAcceptError();
Expand Down Expand Up @@ -291,6 +292,9 @@ abstract contract PartyGovernance is
IProposalExecutionEngine(_GLOBALS.getAddress(LibGlobals.GLOBAL_PROPOSAL_ENGINE_IMPL)),
abi.encode(proposalEngineOpts)
);
if (govOpts.voteDuration < 1 hours) {
revert InvalidGovernanceParameter(govOpts.voteDuration);
}
// Set the governance parameters.
_getSharedProposalStorage().governanceValues = GovernanceValues({
voteDuration: govOpts.voteDuration,
Expand Down
6 changes: 3 additions & 3 deletions test/GasBenchmarks.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ contract GasBenchmarks is SetupPartyHelper {
opts.name = "PARTY";
opts.symbol = "PR-T";
opts.governance.hosts = hosts;
opts.governance.voteDuration = 99;
opts.governance.voteDuration = 1 hours;
opts.governance.executionDelay = _EXECUTION_DELAY;
opts.governance.passThresholdBps = 1000;
opts.governance.totalVotingPower = 301;
Expand All @@ -51,7 +51,7 @@ contract GasBenchmarks is SetupPartyHelper {
opts.name = "PARTY";
opts.symbol = "PR-T";
opts.governance.hosts = hosts;
opts.governance.voteDuration = 99;
opts.governance.voteDuration = 1 hours;
opts.governance.executionDelay = _EXECUTION_DELAY;
opts.governance.passThresholdBps = 1000;
opts.governance.totalVotingPower = 301;
Expand Down Expand Up @@ -227,7 +227,7 @@ contract GasBenchmarks is SetupPartyHelper {
partyOpts.name = "PARTY";
partyOpts.symbol = "PR-T";
partyOpts.governanceOpts.hosts = hosts;
partyOpts.governanceOpts.voteDuration = 99;
partyOpts.governanceOpts.voteDuration = 1 hours;
partyOpts.governanceOpts.executionDelay = _EXECUTION_DELAY;
partyOpts.governanceOpts.passThresholdBps = 1000;
partyOpts.governanceOpts.partyFactory = partyFactory;
Expand Down
2 changes: 1 addition & 1 deletion test/TestUsers.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ contract PartyAdmin is Test {
Party.PartyOptions({
governance: PartyGovernance.GovernanceOpts({
hosts: hosts,
voteDuration: 99,
voteDuration: 1 hours,
executionDelay: 300,
passThresholdBps: partyOpts.passThresholdBps,
totalVotingPower: partyOpts.totalVotingPower,
Expand Down
14 changes: 7 additions & 7 deletions test/crowdfund/AtomicManualParty.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ contract AtomicManualPartyTest is SetupPartyHelper {
Party.PartyOptions memory opts;
opts.name = "PARTY";
opts.symbol = "PR-T";
opts.governance.voteDuration = 99;
opts.governance.voteDuration = 1 hours;
opts.governance.executionDelay = _EXECUTION_DELAY;
opts.governance.passThresholdBps = 1000;
opts.governance.totalVotingPower = 180;
Expand Down Expand Up @@ -100,7 +100,7 @@ contract AtomicManualPartyTest is SetupPartyHelper {
Party.PartyOptions memory opts;
opts.name = "PARTY";
opts.symbol = "PR-T";
opts.governance.voteDuration = 99;
opts.governance.voteDuration = 1 hours;
opts.governance.executionDelay = _EXECUTION_DELAY;
opts.governance.passThresholdBps = 1000;
opts.governance.totalVotingPower = 180;
Expand Down Expand Up @@ -149,7 +149,7 @@ contract AtomicManualPartyTest is SetupPartyHelper {
Party.PartyOptions memory opts;
opts.name = "PARTY";
opts.symbol = "PR-T";
opts.governance.voteDuration = 99;
opts.governance.voteDuration = 1 hours;
opts.governance.executionDelay = _EXECUTION_DELAY;
opts.governance.passThresholdBps = 1000;
opts.governance.totalVotingPower = 180;
Expand Down Expand Up @@ -181,7 +181,7 @@ contract AtomicManualPartyTest is SetupPartyHelper {
Party.PartyOptions memory opts;
opts.name = "PARTY";
opts.symbol = "PR-T";
opts.governance.voteDuration = 99;
opts.governance.voteDuration = 1 hours;
opts.governance.executionDelay = _EXECUTION_DELAY;
opts.governance.passThresholdBps = 1000;
opts.governance.totalVotingPower = 180;
Expand All @@ -206,7 +206,7 @@ contract AtomicManualPartyTest is SetupPartyHelper {
Party.PartyOptions memory opts;
opts.name = "PARTY";
opts.symbol = "PR-T";
opts.governance.voteDuration = 99;
opts.governance.voteDuration = 1 hours;
opts.governance.executionDelay = _EXECUTION_DELAY;
opts.governance.passThresholdBps = 1000;
opts.governance.totalVotingPower = 260;
Expand Down Expand Up @@ -257,7 +257,7 @@ contract AtomicManualPartyTest is SetupPartyHelper {
Party.PartyOptions memory opts;
opts.name = "PARTY";
opts.symbol = "PR-T";
opts.governance.voteDuration = 99;
opts.governance.voteDuration = 1 hours;
opts.governance.executionDelay = _EXECUTION_DELAY;
opts.governance.passThresholdBps = 1000;
opts.governance.totalVotingPower = 180;
Expand Down Expand Up @@ -289,7 +289,7 @@ contract AtomicManualPartyTest is SetupPartyHelper {
Party.PartyOptions memory opts;
opts.name = "PARTY";
opts.symbol = "PR-T";
opts.governance.voteDuration = 99;
opts.governance.voteDuration = 1 hours;
opts.governance.executionDelay = _EXECUTION_DELAY;
opts.governance.passThresholdBps = 1000;
opts.governance.totalVotingPower = 180;
Expand Down
16 changes: 8 additions & 8 deletions test/crowdfund/CrowdfundFactory.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ contract CrowdfundFactoryTest is Test, TestUtils {
partyImpl: party,
partyFactory: partyFactory,
hosts: _toAddressArray(_randomAddress()),
voteDuration: randomUint40,
voteDuration: randomUint40 < 1 hours ? 1 hours : randomUint40,
executionDelay: randomUint40,
passThresholdBps: randomBps,
feeBps: randomBps,
Expand Down Expand Up @@ -223,7 +223,7 @@ contract CrowdfundFactoryTest is Test, TestUtils {
partyImpl: party,
partyFactory: partyFactory,
hosts: _toAddressArray(_randomAddress()),
voteDuration: randomUint40,
voteDuration: randomUint40 < 1 hours ? 1 hours : randomUint40,
executionDelay: randomUint40,
passThresholdBps: randomBps,
feeBps: randomBps,
Expand Down Expand Up @@ -434,7 +434,7 @@ contract CrowdfundFactoryTest is Test, TestUtils {
partyImpl: party,
partyFactory: partyFactory,
hosts: _toAddressArray(_randomAddress()),
voteDuration: randomUint40,
voteDuration: randomUint40 < 1 hours ? 1 hours : randomUint40,
executionDelay: randomUint40,
passThresholdBps: randomBps,
feeBps: randomBps,
Expand Down Expand Up @@ -510,7 +510,7 @@ contract CrowdfundFactoryTest is Test, TestUtils {
partyImpl: party,
partyFactory: partyFactory,
hosts: _toAddressArray(_randomAddress()),
voteDuration: randomUint40,
voteDuration: randomUint40 < 1 hours ? 1 hours : randomUint40,
executionDelay: randomUint40,
passThresholdBps: randomBps,
feeBps: randomBps,
Expand Down Expand Up @@ -589,7 +589,7 @@ contract CrowdfundFactoryTest is Test, TestUtils {
partyImpl: party,
partyFactory: partyFactory,
hosts: _toAddressArray(_randomAddress()),
voteDuration: randomUint40,
voteDuration: randomUint40 < 1 hours ? 1 hours : randomUint40,
executionDelay: randomUint40,
passThresholdBps: randomBps,
feeBps: randomBps,
Expand Down Expand Up @@ -696,7 +696,7 @@ contract CrowdfundFactoryTest is Test, TestUtils {
partyImpl: party,
partyFactory: partyFactory,
hosts: _toAddressArray(_randomAddress()),
voteDuration: randomUint40,
voteDuration: randomUint40 < 1 hours ? 1 hours : randomUint40,
executionDelay: randomUint40,
passThresholdBps: randomBps,
feeBps: randomBps,
Expand Down Expand Up @@ -800,7 +800,7 @@ contract CrowdfundFactoryTest is Test, TestUtils {
partyImpl: party,
partyFactory: partyFactory,
hosts: _toAddressArray(_randomAddress()),
voteDuration: randomUint40,
voteDuration: randomUint40 < 1 hours ? 1 hours : randomUint40,
executionDelay: randomUint40,
passThresholdBps: randomBps,
feeBps: randomBps,
Expand Down Expand Up @@ -967,7 +967,7 @@ contract CrowdfundFactoryTest is Test, TestUtils {
partyImpl: party,
partyFactory: partyFactory,
hosts: _toAddressArray(_randomAddress()),
voteDuration: randomUint40,
voteDuration: randomUint40 < 1 hours ? 1 hours : randomUint40,
executionDelay: randomUint40,
passThresholdBps: randomBps,
feeBps: randomBps,
Expand Down
4 changes: 2 additions & 2 deletions test/party/PartyFactory.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ contract PartyFactoryTest is Test, TestUtils {
Party.PartyOptions memory opts = Party.PartyOptions({
governance: PartyGovernance.GovernanceOpts({
hosts: _toAddressArray(_randomAddress()),
voteDuration: randomUint40,
voteDuration: randomUint40 < 1 hours ? 1 hours : randomUint40,
executionDelay: randomUint40,
passThresholdBps: randomBps,
totalVotingPower: randomUint96,
Expand Down Expand Up @@ -138,7 +138,7 @@ contract PartyFactoryTest is Test, TestUtils {
Party.PartyOptions memory opts = Party.PartyOptions({
governance: PartyGovernance.GovernanceOpts({
hosts: _toAddressArray(_randomAddress()),
voteDuration: randomUint40,
voteDuration: randomUint40 < 1 hours ? 1 hours : randomUint40,
executionDelay: randomUint40,
passThresholdBps: randomBps,
totalVotingPower: randomUint96,
Expand Down
2 changes: 1 addition & 1 deletion test/party/PartyGovernance.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -925,7 +925,7 @@ contract PartyGovernanceTest is Test, TestUtils {

_assertProposalStatus(party, 1, PartyGovernance.ProposalStatus.Voting, 50);

vm.warp(block.timestamp + 98);
vm.warp(block.timestamp + 1 hours - 1);
_assertProposalStatus(party, 1, PartyGovernance.ProposalStatus.Voting, 50);

// ensure defeated
Expand Down
1 change: 1 addition & 0 deletions test/party/PartyGovernanceNFTUnit.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ contract PartyGovernanceNFTUnitTest is TestUtils {
ProposalStorage.ProposalEngineOpts defaultProposalEngineOpts;

function _initGovernance() private {
defaultGovernanceOpts.voteDuration = 1 hours;
defaultGovernanceOpts.totalVotingPower = 1e18;
nft.initialize(
"TEST",
Expand Down
2 changes: 1 addition & 1 deletion test/party/PartyGovernanceUnit.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1485,7 +1485,7 @@ contract PartyGovernanceUnitTest is Test, TestUtils {
uint256[] memory preciousTokenIds
) = _createPreciousTokens(2);
defaultGovernanceOpts.executionDelay = 60;
defaultGovernanceOpts.voteDuration = 61;
defaultGovernanceOpts.voteDuration = 1 hours;
TestablePartyGovernance gov = _createGovernance(
false,
100e18,
Expand Down
2 changes: 1 addition & 1 deletion test/proposals/SetGovernanceParameterProposal.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ contract SetGovernanceParameterProposalTest is SetupPartyHelper {
event ProposalPassed(uint256 indexed proposalId);

uint256 oldPassThresholdBps = 1000;
uint256 oldVoteDuration = 300;
uint256 oldVoteDuration = 1 hours;
uint256 oldExecutionDelay = 99;

function testGovernanceParameterProposal_multiple() public {
Expand Down
2 changes: 1 addition & 1 deletion test/utils/SetupPartyHelper.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ abstract contract SetupPartyHelper is TestUtils, ERC721Receiver {
opts.name = "PARTY";
opts.symbol = "PR-T";
opts.governance.hosts = hosts;
opts.governance.voteDuration = 300;
opts.governance.voteDuration = 1 hours;
opts.governance.executionDelay = _EXECUTION_DELAY;
opts.governance.passThresholdBps = 1000;
opts.proposalEngine.allowArbCallsToSpendPartyEth = true;
Expand Down

0 comments on commit 65334de

Please sign in to comment.