| Audit Date | 12/05/2023 - 12/09/2023 |
| Auditor | HollaDieWaldfee (@HollaWaldfee100) |
| Version 1 | 12/09/2023 Initial Report |
| Version 2 | 12/15/2023 Mitigation Review |
| Version 3 | 12/15/2023 Mitigation Review Update |
- Audit Report - PartyDAO
- Contents
- Disclaimer
- About HollaDieWaldfee
- Scope
- Severity Classification
- Summary
- Code4rena Mitigation Review
- C4-191 - QA Report
- C4-233 - Single host can unfairly skip veto period
- C4-236 - QA Report
- C4-295 - Governance parameters are not cached per proposal
- C4-306 - QA Report
- C4-311 - Unexpected vote delegation
- C4-340 - Faulty implementation of
ERC4906standard - C4-344 - QA Report
- C4-437 - QA Report
- C4-453 - Crowdfund cannot be finalized in some cases
- C4-475 -
ArbitraryCallsProposals can fail - C4-529 - QA Report
- C4-551 - QA Report
- C4-573 - QA Report
- Additional Findings
The following smart contract audit report is based on the information and code provided by the client, and any findings or recommendations are made solely on the basis of this information. While the Auditor has exercised due care and skill in conducting the audit, it cannot be guaranteed that all issues have been identified and that there are no undiscovered errors or vulnerabilities in the code.
Furthermore, this report is not an endorsement or certification of the smart contracts, and the Auditor does not assume any responsibility for any losses or damages that may result from the use of the smart contracts, either in their current form or in any modified version thereof.
HollaDieWaldfee is a top ranked Smart Contract Auditor performing audits on code4rena and Sherlock. He has placed 1st in multiple audit contests.
On Sherlock he uses the handle "roguereddwarf" to compete in audit contests.
He can also be booked for conducting Private Audits.
Contact:
Twitter/X: @HollaWaldfee100
The audit has been conducted on the code in the client's GitHub repository (https://github.com/PartyDAO/party-protocol) at commit eef9ffe2dd0f5b9cb92b1017cff02116b4091d24.
The goal of the audit is to provide a thorough review of the mitigations that have been implemented in response to PartyDAO's latest audit contest in Code4rena.
The findings that have been discovered in the Code4rena audit contest and confirmed by PartyDAO can be seen here.
The approach that is followed for every mitigation is:
- check the mitigation narrowly (is the specific reported scenario fixed?)
- can the mitigation lead to problems?
- can similar issues be found in the codebase?
Apart from assessing the mitigations, the code changes that were subject of the Code4rena contest are assessed more generally, although this review must not be regarded as a full audit of those changes.
The report references each Code4rena issue for which a mitigation has been implemented and discusses the mitigation in detail based on the above approach.
The mitigation review has been conducted at commit 1276eb7bf8f3e36e84f1220a898875ea9b86f692.
Issues that are referenced from the Code4rena audit contest have their severity determined based on the Code4rena Severity Categorization rules.
Additional issues that are discovered have their severity determined based on the following table:
| Severity | Impact: High | Impact: Medium | Impact: Low |
|---|---|---|---|
| Likelihood: High |  |
|
 |
| Likelihood: Medium | |
|
 |
| Likelihood: Low | |
|
|
Impact - the technical, economic and reputation damage of a successful attack
Likelihood - the chance that a particular vulnerability is discovered and exploited
Code4rena report: code-423n4/2023-10-party-findings#191
Note: The report contains several findings, but only those below have been decided to require fixes.
C4-191-L01
The issue has been fixed with regards to InitialETHCrowdfund. It is now clear that only msg.value passed along with the call to initialize() is credited for the initial contribution.
However, the same clarification should be applied to Crowdfund.
Mitigation Review:
The same comment from InitialETHCrowdfund has been applied to Crowdfund.
C4-191-L03
The issue has been fixed with regards to ETHCrowdfundBase. A safe cast is now used.
However, the same unsafe cast exists in two other instances:
It is reommended to use safeCastUint256ToUint40() in these instances as well.
Mitigation Review:
The recommendation has been implemented.
C4-191-L07
The issue is fixed since totalVotes is now cast to uint256 and uint256(totalVotes) * 1e4 is equal to type(uint96).max * 1e4 << type(uint256).max at most.
Beyond the instance highlighted in the report, the client did also fix another intance of the same calculation within the same contract.
During the review, additional instances have been found that could experience overflow issues.
Fixes:
diff --git a/contracts/crowdfund/ETHCrowdfundBase.sol b/contracts/crowdfund/ETHCrowdfundBase.sol
index 880eb24..0964834 100644
--- a/contracts/crowdfund/ETHCrowdfundBase.sol
+++ b/contracts/crowdfund/ETHCrowdfundBase.sol
@@ -300,7 +300,10 @@ abstract contract ETHCrowdfundBase is Implementation {
address payable fundingSplitRecipient_ = fundingSplitRecipient;
uint16 fundingSplitBps_ = fundingSplitBps;
if (fundingSplitRecipient_ != address(0) && fundingSplitBps_ > 0) {
- contribution = (contribution * 1e4) / (1e4 - fundingSplitBps_);
+ /* @audit-info
+ Downcast is safe since `contribution` cannot excced type(uint96).max.
+ When the contribution is made, it cannot exceed type(uint96).max, neither can `totalContributions` exceed it.
+ */
+ contribution = uint96(uint256(contribution) * 1e4) / (1e4 - fundingSplitBps_);
}
return contribution;
}
@@ -311,7 +314,7 @@ abstract contract ETHCrowdfundBase is Implementation {
address payable fundingSplitRecipient_ = fundingSplitRecipient;
uint16 fundingSplitBps_ = fundingSplitBps;
if (fundingSplitRecipient_ != address(0) && fundingSplitBps_ > 0) {
- contribution = (contribution * (1e4 - fundingSplitBps_)) / 1e4;
+ /* @audit-info
+ Safe since contribution initially fits into uint96 and cannot get bigger
+ */
+ contribution = uint96((uint256(contribution) * (1e4 - fundingSplitBps_)) / 1e4);
}
return contribution;
}
@@ -350,7 +353,10 @@ abstract contract ETHCrowdfundBase is Implementation {
address payable fundingSplitRecipient_ = fundingSplitRecipient;
uint16 fundingSplitBps_ = fundingSplitBps;
if (fundingSplitRecipient_ != address(0) && fundingSplitBps_ > 0) {
- totalContributions_ -= (totalContributions_ * fundingSplitBps_) / 1e4;
+ /* @audit-info
+ Assuming fundingSplitBps_ <= 1e4, this cannot overflow uint96
+ */
+ totalContributions_ -= uint96((uint256(totalContributions_) * fundingSplitBps_) / 1e4);
}
// Update the party's total voting power.
@@ -380,7 +386,10 @@ abstract contract ETHCrowdfundBase is Implementation {
fundingSplitPaid = true;
// Transfer funding split to recipient.
- splitAmount = (totalContributions * fundingSplitBps_) / 1e4;
+ /* @audit-info
+ Assuming fundingSplitBps_ <= 1e4, this cannot overflow uint96
+ */
+ splitAmount = uint96((uint256(totalContributions) * fundingSplitBps_) / 1e4);
payable(fundingSplitRecipient_).transferEth(splitAmount);
emit FundingSplitSent(fundingSplitRecipient_, splitAmount);
diff --git a/contracts/signature-validators/OffChainSignatureValidator.sol b/contracts/signature-validators/OffChainSignatureValidator.sol
index f9a54eb..bfafc85 100644
--- a/contracts/signature-validators/OffChainSignatureValidator.sol
+++ b/contracts/signature-validators/OffChainSignatureValidator.sol
@@ -62,7 +62,7 @@ contract OffChainSignatureValidator is IERC1271 {
revert InvalidSignature();
}
- uint96 signerVotingPowerBps = party.getVotingPowerAt(
+ uint256 signerVotingPowerBps = party.getVotingPowerAt(
signer,
uint40(block.timestamp),
type(uint256).maxMitigation Review:
Four of the five fixes have been implemented as recommended, however in two of the instances, the recommended fix was not complete.
The following changes need to be applied (reasoning in the comments, missing instance is included):
diff --git a/contracts/crowdfund/ETHCrowdfundBase.sol b/contracts/crowdfund/ETHCrowdfundBase.sol
index ddcd0fa..890168a 100644
--- a/contracts/crowdfund/ETHCrowdfundBase.sol
+++ b/contracts/crowdfund/ETHCrowdfundBase.sol
@@ -309,7 +309,10 @@ abstract contract ETHCrowdfundBase is Implementation {
// Downcast is safe since `contribution` cannot exceed
// type(uint96).max. When the contribution is made, it cannot exceed
// type(uint96).max, neither can `totalContributions` exceed it.
- contribution = uint96(uint256(contribution) * 1e4) / (1e4 - fundingSplitBps_);
+ /* @audit-info
+ only the result must be cast to uint96, brackets were missing
+ */
+ contribution = uint96((uint256(contribution) * 1e4) / (1e4 - fundingSplitBps_));
}
return contribution;
}
@@ -390,7 +393,10 @@ abstract contract ETHCrowdfundBase is Implementation {
// Transfer funding split to recipient.
// Assuming fundingSplitBps_ <= 1e4, this cannot overflow uint96
address payable fundingSplitRecipient_ = fundingSplitRecipient;
- splitAmount = (totalContributions * fundingSplitBps_) / 1e4;
+ /* @audit-info
+ Assuming fundingSplitBps_ <= 1e4, this cannot overflow uint96
+ */
+ splitAmount = uint96((uint256(totalContributions) * fundingSplitBps_) / 1e4);
payable(fundingSplitRecipient_).transferEth(splitAmount);
emit FundingSplitSent(fundingSplitRecipient_, splitAmount);
diff --git a/contracts/signature-validators/OffChainSignatureValidator.sol b/contracts/signature-validators/OffChainSignatureValidator.sol
index bf38e25..988c689 100644
--- a/contracts/signature-validators/OffChainSignatureValidator.sol
+++ b/contracts/signature-validators/OffChainSignatureValidator.sol
@@ -62,11 +62,14 @@ contract OffChainSignatureValidator is IERC1271 {
revert InvalidSignature();
}
- uint256 signerVotingPowerBps = party.getVotingPowerAt(
+ /* @audit-info
+ without the additional cast, the calculation is still done with uint96 types and only later assigned to uint256
+ */
+ uint256 signerVotingPowerBps = uint256(party.getVotingPowerAt(
signer,
uint40(block.timestamp),
type(uint256).max
- ) * 10000;
+ )) * 10000;
if (signerVotingPowerBps == 0 && party.balanceOf(signer) == 0) {
// Must own a party card or be delegated voting powerUpdate: The recommendation from the mitigation review has been implemented in commit 88b2308cc15adf26e6b23033c0b0a715842adb00.
C4-191-R01
The InitialETHCrowdfund.batchContribute() and InitialETHCrowdfund.batchContributeFor() functions now treat msg.value equally.
In addition, it has been checked that all crowdfunds exhibit the same behavior.
C4-191-N02
The comment has been fixed.
Searching for other incorrect comments is beyond the scope of this review.
C4-191-N03
The comment has been fixed.
Searching for other incorrect comments is beyond the scope of this review.
Code4rena report: code-423n4/2023-10-party-findings#233
Note: Issues #322 and #538 that were marked as "sponsor confirmed" are a duplicate of #233.
The issue has been fixed by making host privileges non-transferrable.
Hosts are only set once upon initialization of the party and can then only give up their privileges but not transfer them to another address.
All the slightly different attack scenarios that are discussed in #233 and its duplicates are thereby prevented.
Beyond that, it's possible to simplify the PartyGovernance.abdicateHost() function:
diff --git a/contracts/party/PartyGovernance.sol b/contracts/party/PartyGovernance.sol
index f4c1595..818efcf 100644
--- a/contracts/party/PartyGovernance.sol
+++ b/contracts/party/PartyGovernance.sol
@@ -452,19 +452,11 @@ abstract contract PartyGovernance is
}
/// @notice Transfer party host status to another.
- /// @param newPartyHost The address of the new host.
- function abdicateHost(address newPartyHost) external {
+ function abdicateHost() external {
_assertHost();
- // 0 is a special case burn address.
- if (newPartyHost != address(0)) {
- // Can only abdicate host
- revert InvalidNewHostError();
- } else {
- // Burned the host status
- --numHosts;
- }
+ --numHosts;
isHost[msg.sender] = false;
- emit HostStatusTransferred(msg.sender, newPartyHost);
+ emit HostStatusTransferred(msg.sender, address(0));
}Mitigation Review:
The recommendation has been implemented.
Update: The simplification has been reverted in commit e35a0b50257d1b1df522de8f974dc58a04c38de9 to keep the ABI unchanged.
Code4rena report: code-423n4/2023-10-party-findings#236
Note: The report contains several findings, but only those below have been decided to require fixes.
C4-236-L06
authorities are fully trusted and so being able for them to burn party cards is safe.
The comment that authorities can only burn party cards before the party has started is removed in one place.
However, there exists a second instance of the same comment that should be removed:
diff --git a/contracts/party/PartyGovernanceNFT.sol b/contracts/party/PartyGovernanceNFT.sol
index c084784..20ba97e 100644
--- a/contracts/party/PartyGovernanceNFT.sol
+++ b/contracts/party/PartyGovernanceNFT.sol
@@ -326,8 +326,7 @@ contract PartyGovernanceNFT is PartyGovernance, ERC721, IERC2981 {
emit BatchMetadataUpdate(0, type(uint256).max);
}
- /// @notice Burn governance NFT and remove its voting power. Can only be
- /// called by an authority before the party has started.
+ /// @notice Burn governance NFT and remove its voting power.
/// @param tokenId The ID of the governance NFTs to burn.
function burn(uint256 tokenId) external {
uint256[] memory tokenIds = new uint256[](1);Mitigation Review:
The recommendation has been implemented.
C4-236-L07
Fixed, see comment for C4-191-L01.
C4-236-N01
This finding is fixed. The comment that the gatekeeper is set up in the ETHCrowdfundBase._initialize() function has been removed.
The gatekeeper is correctly set up in the upstream InitialETHCrowdfund.initialize() function.
C4-236-N02
The typo has been fixed.
Searching for other typos is beyond the scope of this review.
Code4rena report: code-423n4/2023-10-party-findings#295
Note: Issue #413 that was marked as "sponsor confirmed" is a duplicate of #295.
The issue has been fixed successfully by caching the governance parameters that can be changed via SetGovernanceParameterProposal (voteDuration, executionDelay, passThresholdBps) for each proposal.
No further issues / improvements have been found in the implementation of the fix.
Code4rena report: code-423n4/2023-10-party-findings#306
Note: The report contains several findings, but only those below have been decided to require fixes.
C4-306-QA2
The same validation on the voteDuration parameter that exists in SetGovernanceParameterProposal._executeSetGovernanceParameter() is now added to the PartyGovernance._initialize() function.
Since these are the only two places where voteDuration can change, the check is now consistent throughout the whole application.
The report suggests to add consistent validation to all governance parameters.
The SetGovernanceParameterProposal._executeSetGovernanceParameter() function checks that 0 < executionDelay <= 30 days and 0 < passThresholdBps <= 10000.
The executionDelay check is missing entirely from PartyGovernance._initialize(). And for passThresholdBps, the check that it must be greater zero is missing.
Therefore consider adding the missing checks for executionDelay and passThresholdBps.
Mitigation Review:
It is now checked that executionDelay <= 30 days.
There is still no check that passThresholdBps and executionDelay must have some minimum value.
Consider adding sanity checks that these must be greater zero.
Update: The zero address checks have been implemented in commit 4249514a3b848cf8350e84d15f8478379811ed5b.
C4-306-QA3
The missing zero address check has been added to ETHCrowdfundBase._initialize().
With the knowledge that fundingSplitsBps > 0 => fundingSplitRecipient != address(0), we can further simplify the code:
diff --git a/contracts/crowdfund/ETHCrowdfundBase.sol b/contracts/crowdfund/ETHCrowdfundBase.sol
index 880eb24..b31d2c4 100644
--- a/contracts/crowdfund/ETHCrowdfundBase.sol
+++ b/contracts/crowdfund/ETHCrowdfundBase.sol
@@ -297,9 +297,8 @@ abstract contract ETHCrowdfundBase is Implementation {
}
function _addFundingSplitToContribution(uint96 contribution) internal view returns (uint96) {
- address payable fundingSplitRecipient_ = fundingSplitRecipient;
uint16 fundingSplitBps_ = fundingSplitBps;
- if (fundingSplitRecipient_ != address(0) && fundingSplitBps_ > 0) {
+ if (fundingSplitBps_ > 0) {
contribution = (contribution * 1e4) / (1e4 - fundingSplitBps_);
}
return contribution;
@@ -308,9 +307,8 @@ abstract contract ETHCrowdfundBase is Implementation {
function _removeFundingSplitFromContribution(
uint96 contribution
) internal view returns (uint96) {
- address payable fundingSplitRecipient_ = fundingSplitRecipient;
uint16 fundingSplitBps_ = fundingSplitBps;
- if (fundingSplitRecipient_ != address(0) && fundingSplitBps_ > 0) {
+ if (fundingSplitBps_ > 0) {
contribution = (contribution * (1e4 - fundingSplitBps_)) / 1e4;
}
return contribution;
@@ -347,9 +345,8 @@ abstract contract ETHCrowdfundBase is Implementation {
delete expiry;
// Transfer funding split to recipient if applicable.
- address payable fundingSplitRecipient_ = fundingSplitRecipient;
uint16 fundingSplitBps_ = fundingSplitBps;
- if (fundingSplitRecipient_ != address(0) && fundingSplitBps_ > 0) {
+ if (fundingSplitBps_ > 0) {
totalContributions_ -= (totalContributions_ * fundingSplitBps_) / 1e4;
}
@@ -371,12 +368,12 @@ abstract contract ETHCrowdfundBase is Implementation {
if (fundingSplitPaid) revert FundingSplitAlreadyPaidError();
- address payable fundingSplitRecipient_ = fundingSplitRecipient;
uint16 fundingSplitBps_ = fundingSplitBps;
- if (fundingSplitRecipient_ == address(0) || fundingSplitBps_ == 0) {
+ if (fundingSplitBps_ == 0) {
revert FundingSplitNotConfiguredError();
}
+ address payable fundingSplitRecipient_ = fundingSplitRecipient;
fundingSplitPaid = true;
// Transfer funding split to recipientMitigation Review:
The recommendation has been implemented.
C4-306-QA4
The ETHCrowdfundBase contract has been declared abstract as recommended.
The same argument can be made for other contracts in the repository like CrowdfundNFT and PartyGovernanceNFT.
If this is a convention that the client wants to implement throughout the whole codebase, all contracts should be checked whether they can be declared abstract.
Mitigation Review:
The two contracts that were mentioned are now abstract.
It is possible that more contracts can be declared abstract. Given that this is of minor importance, this was not explicitly checked.
C4-306-QA5
Fixed, the Natspec comment has been updated.
Searching for other incorrect comments is beyond the scope of this review.
C4-306-QA7
The array length checks have been successfully added to the InitialETHCrowdfund.batchContribute() function.
There are some additional suggestions:
- in
InitialETHCrowdfund.batchContributeFor(), check that array length ofrecipientsmatches as well - implement the same behavior in
Crowdfund.batchContributeFor() - don't cache all array lengths
diff --git a/contracts/crowdfund/Crowdfund.sol b/contracts/crowdfund/Crowdfund.sol
index 78eab6b..03eb345 100644
--- a/contracts/crowdfund/Crowdfund.sol
+++ b/contracts/crowdfund/Crowdfund.sol
@@ -111,6 +111,7 @@ abstract contract Crowdfund is Implementation, ERC721Receiver, CrowdfundNFT {
error BelowMinimumContributionsError(uint96 contributions, uint96 minContributions);
error AboveMaximumContributionsError(uint96 contributions, uint96 maxContributions);
error InvalidMessageValue();
+ error ArityMismatch();
event Burned(address contributor, uint256 ethUsed, uint256 ethOwed, uint256 votingPower);
event Contributed(
@@ -373,6 +374,12 @@ abstract contract Crowdfund is Implementation, ERC721Receiver, CrowdfundNFT {
uint96[] memory values,
bytes[] memory gateDatas
) external payable {
+ uint256 numRecipients = recipients.length;
+
+ if (numRecipients != initialDelegates.length || numRecipients != values.length || numRecipients != gateDatas.length) {
+ revert ArityMismatch();
+ }
+
uint256 valuesSum;
for (uint256 i; i < recipients.length; ++i) {
_setDelegate(recipients[i], initialDelegates[i]);
diff --git a/contracts/crowdfund/InitialETHCrowdfund.sol b/contracts/crowdfund/InitialETHCrowdfund.sol
index a5202bc..82972b4 100644
--- a/contracts/crowdfund/InitialETHCrowdfund.sol
+++ b/contracts/crowdfund/InitialETHCrowdfund.sol
@@ -207,10 +207,8 @@ contract InitialETHCrowdfund is ETHCrowdfundBase {
BatchContributeArgs calldata args
) external payable onlyDelegateCall returns (uint96[] memory votingPowers) {
uint256 numContributions = args.tokenIds.length;
- uint256 numValues = args.values.length;
- uint256 numGateDatas = args.gateDatas.length;
- if (numContributions != numValues || numContributions != numGateDatas) {
+ if (numContributions != args.values.length || numContributions != args.gateDatas.length) {
revert ArityMismatch();
}
@@ -265,14 +263,11 @@ contract InitialETHCrowdfund is ETHCrowdfundBase {
BatchContributeForArgs calldata args
) external payable onlyDelegateCall returns (uint96[] memory votingPowers) {
uint256 numContributions = args.tokenIds.length;
- uint256 numValues = args.values.length;
- uint256 numGateDatas = args.gateDatas.length;
-
- if (numContributions != numValues || numContributions != numGateDatas) {
+ if (numContributions != args.values.length || numContributions != args.gateDatas.length || numContributions != args.recipients.length) {
revert ArityMismatch();
}
- votingPowers = new uint96[](args.recipients.length);
+ votingPowers = new uint96[](numContributions);
uint256 valuesSum;
for (uint256 i; i < numContributions; ++i) {Mitigation Review:
The recommendations have been implemented.
Code4rena report: code-423n4/2023-10-party-findings#311
This finding has been addressed by introducing additional documentation.
(The comment is still missing here though)
The delegate that is passed to the InitialETHCrowdfund's functions only sets the delegate in the Party when it's the first ever mint for the user.
Documenting this can only be regarded as a partial fix.
The larger problem that users can be front-run, thereby ending up delegating their voting power unintentionally, is not addressed.
Even if users are aware of the risk, check their delegate after making / receiving contributions and call PartyGovernance.delegateVotingPower() if needed, there can still be a period of time in which the voting power is unintentionally delegated.
According to the sponsor's comment, they intentionally did not provide a more elaborate solution.
Therefore this issue is marked as "Acknowledged".
Given that the delegate logic in ETHCrowdfundBase and InitialETHCrowdfund has no effect, it can be removed. All that needs to remain is passing the initialDelegate to the call to PartyGovernanceNFT.mint().
Mitigation Review.
The missing comment has been added.
Code4rena report: code-423n4/2023-10-party-findings#340
The report describes a lack of ERC4906 compliance due to not emitting the required events when the voting power is changed.
However, accounting for changes in the voting power is not sufficient to provide ERC4906 compliance.
Party ERC4906 compliance considerations
Let's assume party cards use the standard PartyNFTRenderer to get tokenURIs.
By checking the tokenURI() function we can see the information that goes into the URI for any token. If any of that information changes, a MetadataUpdate event or, if multiple tokens are affected, a BatchMetadataUpdate event must be emitted.
We see that apart from information contained within the Party (total voting power, voting power of token, name, ...) the token URIs downstream depend on MetadataProvider.
Compliance with ERC4906 requires that whenever the MetadataProvider sets the metadata, the respective event must be emitted as well. Currently there is no mechanism by which the MetadataProvider can trigger the event.
A similar problem exists with distributions. Whenever a token claims his distribution, the metadata changes, requiring that an event is emitted. An event is currently only emitted in the PartyGovernance.distribute() function when the distribution is created, but no event is emitted when the distribution is claimed.
As a result, Parties are not ERC4906 compliant.
Crowdfund ERC4906 compliance considerations
The token URIs for CrowdfundNFTs are constructed in a simpler way.
The only variables that go into the tokenURI and can change over time are contribution amount and Crowdfund status.
Recommendation
If Party NFTs and Crowdfund NFTs should be ERC4906 compliant, all variables that are used to construct the tokenURI need to be mapped out and it must be ensured that all of their state transition trigger the respective event.
Another consideration is that for Party NFTs, the tokenCount variable can be used to restrict the _toTokenId parameter in the BatchMetadataUpdate event as only token IDs in the range [1,tokenCount] exist.
Token IDs for Crowdfund NFTs are the addresses of the participants and so this optimization isn't possible there.
Implementing ERC4906 compliance requires additional code / complexity (-> e.g., MetadataProvider must be able to trigger event) and it should be assessed whether ERC4906 compliance is really necessary.
Mitigation Review
During further discussion with the client, it has been assessed that the MetadataUpdate and BatchMetadataUpdate events are only needed for OpenSea to update the metadata.
OpenSea does not check the supportsInterface() function and so it's best to remove ERC4906 from supportsInterface() to not suggest full ERC4906 compliance while still making use of metadata updates on OpenSea.
diff --git a/contracts/crowdfund/CrowdfundNFT.sol b/contracts/crowdfund/CrowdfundNFT.sol
index 61424cf..b179237 100644
--- a/contracts/crowdfund/CrowdfundNFT.sol
+++ b/contracts/crowdfund/CrowdfundNFT.sol
@@ -95,9 +95,7 @@ abstract contract CrowdfundNFT is IERC721, IERC4906, EIP165, ReadOnlyDelegateCal
return
super.supportsInterface(interfaceId) ||
// ERC721 interface ID
- interfaceId == 0x80ac58cd ||
- // ERC4906 interface ID
- interfaceId == 0x49064906;
+ interfaceId == 0x80ac58cd;
}
/// @notice Returns a URI to render the NFT.
diff --git a/contracts/party/PartyGovernance.sol b/contracts/party/PartyGovernance.sol
index 19f519f..af519d1 100644
--- a/contracts/party/PartyGovernance.sol
+++ b/contracts/party/PartyGovernance.sol
@@ -346,9 +346,7 @@ abstract contract PartyGovernance is
function supportsInterface(bytes4 interfaceId) public pure virtual returns (bool) {
return
interfaceId == type(IERC721Receiver).interfaceId ||
- interfaceId == type(ERC1155TokenReceiverBase).interfaceId ||
- // ERC4906 interface ID
- interfaceId == 0x49064906;
+ interfaceId == type(ERC1155TokenReceiverBase).interfaceId;
}
/// @notice Get the current `ProposalExecutionEngine` instance.Update: The recommendation from the mitigation review has been implemented in commit 6472b5f5acb0a3d2fef9eb7569f7793b48d991d7 and commit 0b84228ed614acb62ebcbcd8fba627a80ccadff2.
Code4rena report: code-423n4/2023-10-party-findings#344
Note: The report contains several findings, but only those below have been decided to require fixes.
C4-344-L07
Fixed, see comment for C4-573.
C4-344-I02
Fixed, see comment for C4-191-L01.
C4-344-I04
Fixed, see comment for C4-236-N02.
Code4rena report: code-423n4/2023-10-party-findings#437
The report describes a scenario where the amount parameter emitted in the Contributed event is wrong.
Specifically, this could occur when there was a refund.
The fix has removed the refund functionality and instead reverts when newTotalContributions > maxTotalContributions_.
Hence, the value emitted in the Contributed event is always correct now.
Code4rena report: code-423n4/2023-10-party-findings#453
With the fix applied, the InitialETHCrowdfund calls finalize() when a contribution is made in such a way that another contribution of minContribution amount wouldn't be possible anymore.
if (
maxTotalContributions_ == newTotalContributions ||
minContribution_ > maxTotalContributions_ - newTotalContributions
) {
_finalize(newTotalContributions);
}A downside of this fix is that crowdfunds can close with less than minTotalContributions contributions which was not possible before.
Assume the following scenario:
maxTotalContributions = 10 ETH
minTotalContributions = 9 ETH
minContribution = 2 ETH
When contributions are made to reach >8 ETH, finalize() is called without necessarily reaching minTotalContributions
This can be fixed in the following way:
diff --git a/contracts/crowdfund/ETHCrowdfundBase.sol b/contracts/crowdfund/ETHCrowdfundBase.sol
index 880eb24..bfb8f40 100644
--- a/contracts/crowdfund/ETHCrowdfundBase.sol
+++ b/contracts/crowdfund/ETHCrowdfundBase.sol
@@ -65,6 +65,7 @@ abstract contract ETHCrowdfundBase is Implementation {
error InvalidDelegateError();
error NotEnoughContributionsError(uint96 totalContribution, uint96 minTotalContributions);
error MinGreaterThanMaxError(uint96 min, uint96 max);
+ error MinMaxDifferenceTooSmall(uint96 min, uint96 max);
error MaxTotalContributionsCannotBeZeroError(uint96 maxTotalContributions);
error BelowMinimumContributionsError(uint96 contributions, uint96 minContributions);
error AboveMaximumContributionsError(uint96 contributions, uint96 maxContributions);
@@ -150,8 +151,8 @@ abstract contract ETHCrowdfundBase is Implementation {
minContribution = opts.minContribution;
maxContribution = opts.maxContribution;
// Set the min total contributions.
- if (opts.minTotalContributions > opts.maxTotalContributions) {
- revert MinGreaterThanMaxError(opts.minTotalContributions, opts.maxTotalContributions);
+ if (opts.maxTotalContributions - opts.minTotalContributions + 1 < opts.minContribution) {
+ revert MinMaxDifferenceTooSmall(opts.minTotalContributions, opts.maxTotalContributions);
}
minTotalContributions = opts.minTotalContributions;
// Set the max total contributions.The fix ensures that when minTotalContributions hasn't been reached yet, it's always possible to make another contribution or multiple contributions to reach minTotalContributions.
Note that by implementing this fix, minContribution must always be <2 when minTotalContributions == maxTotalContributions.
Another finding from the Code4rena contest, #119, describes a similar issue in terms of impact.
The client has marked it as "sponsor acknowledged", indicating that no fix will be made.
However, since the impact is similar as in #453, which has been decided to require a fix, #119 should be fixed as well.
The most straightforward fix is to check upon initialization of ETHCrowdfundBase that the voting power that one receives from a contribution of size minContribution is not equal to zero.
diff --git a/contracts/crowdfund/ETHCrowdfundBase.sol b/contracts/crowdfund/ETHCrowdfundBase.sol
index 880eb24..0e87608 100644
--- a/contracts/crowdfund/ETHCrowdfundBase.sol
+++ b/contracts/crowdfund/ETHCrowdfundBase.sol
@@ -177,6 +177,10 @@ abstract contract ETHCrowdfundBase is Implementation {
}
// Set whether to disable contributing for existing card.
disableContributingForExistingCard = opts.disableContributingForExistingCard;
+
+ if (convertContributionToVotingPower(opts.minContribution) == 0) {
+ revert ZeroVotingPowerError();
+ }
}
/// @notice Get the current lifecycle of the crowdfund.This fix only works in conjunction with the additional fix for #453.
A downside of this fix is that some crowdfund configurations with minTotalContributions == maxTotalContributions are no longer allowed, as that would require minContribution < 2 which might fail this additional check.
Mitigation Review:
The recommendations have been implemented.
Code4rena report: code-423n4/2023-10-party-findings#475
The issue has been fixed successfully by declaring the ProposalExecutionEngine.executeProposal() function payable. Sending ETH along with executing a proposal is now possible.
The codebase has been checked for similar issues, none have been found.
Code4rena report: code-423n4/2023-10-party-findings#529
The fix ensures that rage quits cannot be enabled when distributions don't require votes.
This prevents the particular attack path outlined in #529, but it fails to address the underlying flaw in the rage quit mechanism that is well described in #237 which is a vulnerability irrespective of the path in #529.
This lack of slippage protection can be solved as follows:
diff --git a/contracts/party/PartyGovernanceNFT.sol b/contracts/party/PartyGovernanceNFT.sol
index c084784..06ca4ed 100644
--- a/contracts/party/PartyGovernanceNFT.sol
+++ b/contracts/party/PartyGovernanceNFT.sol
@@ -451,14 +451,14 @@ contract PartyGovernanceNFT is PartyGovernance, ERC721, IERC2981 {
}
}
- if (amount > 0) {
- uint256 minAmount = minWithdrawAmounts[i];
+ uint256 minAmount = minWithdrawAmounts[i];
- // Check amount is at least minimum.
- if (amount < minAmount) {
- revert BelowMinWithdrawAmountError(amount, minAmount);
- }
+ // Check amount is at least minimum.
+ if (amount < minAmount) {
+ revert BelowMinWithdrawAmountError(amount, minAmount);
+ }
+ if (amount > 0) {
// Transfer token from party to recipient.
if (address(token) == ETH_ADDRESS) {
payable(receiver).transferEth(amount);Mitigation Review:
The recommendation has been implemented.
Code4rena report: code-423n4/2023-10-party-findings#551
The source of the issue was the following if-statement:
if (
thresholdBps == 0 ||
(signerVotingPowerBps > totalVotingPower &&
signerVotingPowerBps / totalVotingPower >= thresholdBps)
) {
return IERC1271.isValidSignature.selector;
}If thresholdBps > 0, we only return IERC1271.isValidSignature.selector, indicating a valid signature, when signerVotingPowerBps > totalVotingPower && signerVotingPowerBps / totalVotingPower >= thresholdBps.
The first condition ensures that the signer has more than 1 Bps = 1 / 10,000 of the total voting power.
The second condition ensures that the signer has at least as much voting power such as to meet thresholdBps.
An edge case was possible where thresholdBps = 1 and the signer passes the second check but doesn't pass the first check. This happens when the signer has exactly 1 / 10,000 of the total voting power.
It was determined that in this case 1 / 10,000 of the total voting power is sufficient and the if-statement was modified like so:
if (
thresholdBps == 0 ||
(signerVotingPowerBps >= totalVotingPower &&
signerVotingPowerBps / totalVotingPower >= thresholdBps)
) {
return IERC1271.isValidSignature.selector;
}Note that the cases in which signerVotingPowerBps >= totalVotingPower is false is a subset of the cases when signerVotingPowerBps / totalVotingPower >= thresholdBps is false.
We know that thresholdBps > 0, otherwise the if-condition is true by default, and so when the signer doesn't have 1 / 10,000 of the total voting power, then he also doesn't have thresholdBps / 10,000 of the total voting power.
Hence, the if-statement can be simplified like so:
diff --git a/contracts/signature-validators/OffChainSignatureValidator.sol b/contracts/signature-validators/OffChainSignatureValidator.sol
index f9a54eb..0375159 100644
--- a/contracts/signature-validators/OffChainSignatureValidator.sol
+++ b/contracts/signature-validators/OffChainSignatureValidator.sol
@@ -78,9 +78,7 @@ contract OffChainSignatureValidator is IERC1271 {
// Either threshold is 0 or signer votes above threshold
if (
- thresholdBps == 0 ||
- (signerVotingPowerBps >= totalVotingPower &&
- signerVotingPowerBps / totalVotingPower >= thresholdBps)
+ thresholdBps == 0 || (signerVotingPowerBps / totalVotingPower >= thresholdBps)
) {
return IERC1271.isValidSignature.selector;
}Mitigation Review:
The recommendations have been implemented.
Code4rena report: code-423n4/2023-10-party-findings#573
It was not checked whether the ecrecover() function returns address(0), which it does for invalid signatures.
This was only a QA finding as an invalid signature would cause a revert in the subsequent check.
address(0) cannot be the owner of Party cards (as ERC721 reverts when minting / transferring NFTs to address(0)) and delegations of voting power to address(0) aren't possible either.
What's important to mention about the ecrecover() function is that it accepts malleable signatures, i.e., signatures that can be changed while remaining valid.
This is why OZ's ECDSA library checks that the signature is not malleable.
The client has explained that in case a signature can be replayed this would be an issue in the third party application using the OffChainSignatureValidator.isValidSignature() function. The third party is supposed to implement a nonce that ensures a signature can't be used multiple times.
Keeping in mind the intended use of the OffChainSignatureValidator.isValidSignature() function, the issue is fixed.
Description:
Some of the changes that have been audited in the Code4rena audit relate to additional privileges of authorities.
Voting power is now more dynamic. It can increase and decrease. Authority contracts can be added to parties that implement additional logic and modify the total voting power or voting power of party cards.
There exists a check in the PartyGovernance.accept() function that prevents a vulnerability that occurs when a user decreases the totalVotingPower via a rage quit.
The same scenario is now possible via authority contracts (depending on the specific logic that they implement).
Impact:
There exists a comment that already explains the attack and its impact:
// Prevent voting in the same block as the last rage quit timestamp.
// This is to prevent an exploit where a member can rage quit to reduce
// the total voting power of the party, then propose and vote in the
// same block since `getVotingPowerAt()` uses `values.proposedTime - 1`.
// This would allow them to use the voting power snapshot just before
// their card was burned to vote, potentially passing a proposal that
// would have otherwise not passed.Recommendation:
Instead of checking when the last raqe quit was, it should be checked when the last modification of totalVotingPower was.
diff --git a/contracts/party/PartyGovernance.sol b/contracts/party/PartyGovernance.sol
index f4c1595..9f2ced4 100644
--- a/contracts/party/PartyGovernance.sol
+++ b/contracts/party/PartyGovernance.sol
@@ -187,7 +187,7 @@ abstract contract PartyGovernance is
error InvalidGovernanceParameter(uint256 value);
error DistributionsRequireVoteError();
error PartyNotStartedError();
- error CannotRageQuitAndAcceptError();
+ error CannotModifyTotalVotingPowerAndAcceptError();
error TooManyHosts();
uint256 private constant UINT40_HIGH_BIT = 1 << 39;
@@ -204,7 +204,7 @@ abstract contract PartyGovernance is
/// @notice Distribution fee recipient.
address payable public feeRecipient;
/// @notice The timestamp of the last time `rageQuit()` was called.
- uint40 public lastRageQuitTimestamp;
+ uint40 public lastTotalVotingPowerChangeTimestamp;
/// @notice The hash of the list of precious NFTs guarded by the party.
bytes32 public preciousListHash;
/// @notice The last proposal ID that was used. 0 means no proposals have been made.
@@ -617,8 +617,8 @@ abstract contract PartyGovernance is
// This would allow them to use the voting power snapshot just before
// their card was burned to vote, potentially passing a proposal that
// would have otherwise not passed.
- if (lastRageQuitTimestamp == block.timestamp) {
- revert CannotRageQuitAndAcceptError();
+ if (lastTotalVotingPowerChangeTimestamp == block.timestamp) {
+ revert CannotModifyTotalVotingPowerAndAcceptError();
}
// Cannot vote twice.
diff --git a/contracts/party/PartyGovernanceNFT.sol b/contracts/party/PartyGovernanceNFT.sol
index c084784..b537bc1 100644
--- a/contracts/party/PartyGovernanceNFT.sol
+++ b/contracts/party/PartyGovernanceNFT.sol
@@ -260,6 +260,7 @@ contract PartyGovernanceNFT is PartyGovernance, ERC721, IERC2981 {
function increaseTotalVotingPower(uint96 votingPower) external {
_assertAuthority();
_getSharedProposalStorage().governanceValues.totalVotingPower += votingPower;
+ lastTotalVotingPowerChangeTimestamp == uint40(block.timestamp);
// Notify third-party platforms that the party NFT metadata has updated
// for all tokens.
@@ -272,6 +273,7 @@ contract PartyGovernanceNFT is PartyGovernance, ERC721, IERC2981 {
function decreaseTotalVotingPower(uint96 votingPower) external {
_assertAuthority();
_getSharedProposalStorage().governanceValues.totalVotingPower -= votingPower;
+ lastTotalVotingPowerChangeTimestamp == uint40(block.timestamp);
// Notify third-party platforms that the party NFT metadata has updated
// for all tokens.
@@ -396,9 +398,7 @@ contract PartyGovernanceNFT is PartyGovernance, ERC721, IERC2981 {
// Used as a reentrancy guard. Will be updated back after ragequit.
rageQuitTimestamp = DISABLE_RAGEQUIT_PERMANENTLY;
- // Update last rage quit timestamp.
- lastRageQuitTimestamp = uint40(block.timestamp);
-
+ lastTotalVotingPowerChangeTimestamp == uint40(block.timestamp);
// Sum up total amount of each token to withdraw.
uint256[] memory withdrawAmounts = new uint256[](withdrawTokens.length);
{Caveat: Depending on the authority contracts in use, this might allow for a DOS attack in which an attacker continuously changes the totalVotingPower such as to prevent proposals from being proposed or votes from being cast.
The ideal solution would be to snapshot the totalVotingPower.
Mitigation Review:
The client decided to implement the recommendation as provided in the above diff.
However, they did forget to set the lastTotalVotingPowerChangeTimestamp variable in the PartyGovernanceNFT.decreaseTotalVotingPower() function.
This means that the issue is still not fixed.
Apply the following change to fix the issue:
diff --git a/contracts/party/PartyGovernanceNFT.sol b/contracts/party/PartyGovernanceNFT.sol
index c23293b..4bbbbdf 100644
--- a/contracts/party/PartyGovernanceNFT.sol
+++ b/contracts/party/PartyGovernanceNFT.sol
@@ -273,6 +273,7 @@ abstract contract PartyGovernanceNFT is PartyGovernance, ERC721, IERC2981 {
function decreaseTotalVotingPower(uint96 votingPower) external {
_assertAuthority();
_getSharedProposalStorage().governanceValues.totalVotingPower -= votingPower;
+ lastTotalVotingPowerChangeTimestamp == uint40(block.timestamp);
// Notify third-party platforms that the party NFT metadata has updated
// for all tokens.Update: The recommendation from the mitigation review has been implemented in commit 27544ea6496b1bd2f4f332734412c21ab4881c00. The issue is now fixed.
Description:
The PartyGovernanceNFT.decreaseTotalVotingPower() function can only be called by authorities which are fully trusted.
Still, it should ideally check that totalVotingPower cannot be decreased below mintedVotingPower since an equal check is included in the PartyGovernanceNFT.increaseVotingPower() function.
Recommendation:
The check wasn't included due to contract size restrictions.
Due to the recommendations in this report, the contract size might be lowered sufficiently to allow for this check now:
diff --git a/contracts/party/PartyGovernanceNFT.sol b/contracts/party/PartyGovernanceNFT.sol
index c084784..04b52f5 100644
--- a/contracts/party/PartyGovernanceNFT.sol
+++ b/contracts/party/PartyGovernanceNFT.sol
@@ -271,7 +271,14 @@ contract PartyGovernanceNFT is PartyGovernance, ERC721, IERC2981 {
/// @param votingPower The new total voting power to add.
function decreaseTotalVotingPower(uint96 votingPower) external {
_assertAuthority();
- _getSharedProposalStorage().governanceValues.totalVotingPower -= votingPower;
+
+ uint96 newTotalVotingPower_ = _getSharedProposalStorage().governanceValues.totalVotingPower - votingPower;
+ uint96 mintedVotingPower_ = mintedVotingPower;
+ if (newTotalVotingPower_ < mintedVotingPower_) {
+ newTotalVotingPower_ = mintedVotingPower_;
+ }
+
+ _getSharedProposalStorage().governanceValues.totalVotingPower = newTotalVotingPower_;
// Notify third-party platforms that the party NFT metadata has updated
// for all tokens.Mitigation Review:
Due to contract size requirements it has been decided that the recommendation won't be applied.
Authorities are fully trusted and so the missing check is only an optional improvement, not a necessity.
HOLLA-3 - Consider implementing equivalence of isUnanimousVotes and hostsAccepted in ArbitraryCallsProposal and ListOnOpenseaAdvancedProposal 
Description:
In the PartyGovernance contract, proposals for which isUnanimousVotes == true or hostsAccepted == true, can skip the execution delay.
The ArbitraryCallsProposal and ListOnOpenseaAdvancedProposal also make use of isUnanimousVotes, granting extra privileges to proposals for which isUnanimousVotes == true. The additional privileges are with regards to "precious tokens".
They don't however check for hostsAccepted which was introduced more recently.
Recommendation:
The recommendation is to check in ArbitraryCallsProposal and ListOnOpenseaAdvancedProposal whether a proposal with hostsAccepted == true should be treated equally to a proposal with isUnanimousVotes == true.
Mitigation Review:
isUnanimousVotes and hostsAccepted are not supposed to be equals.
The recommendation is invalid.