Security Audit
October 31, 2022
Version 1.0.0
Presented by 0xMacro
This document includes the results of the security audit for PartyDAO's smart contract code as found in the section titled ‘Source Code’. The security audit was performed by the Macro security team from September 26, 2022 to October 21, 2022. And, is the 3rd audit on these contracts.
The purpose of this audit is to review the source code of certain PartyDAO Solidity contracts, and provide feedback on the design, architecture, and quality of the source code with an emphasis on validating the correctness and security of the software in its entirety.
Disclaimer: While Macro’s review is comprehensive and has surfaced some changes that should be made to the source code, this audit should not solely be relied upon for security, as no single audit is guaranteed to catch all possible bugs.
The following is an aggregation of issues found by the Macro Audit team:
Severity | Count | Acknowledged | Won't Do | Addressed |
---|---|---|---|---|
High | 1 | - | - | 1 |
Medium | 2 | - | - | 2 |
Low | 1 | - | - | 1 |
Code Quality | 9 | 4 | - | 5 |
Informational | 1 | - | - | 1 |
Gas Optimization | 2 | 2 | - | - |
PartyDAO was quick to respond to these issues.
Our understanding of the specification was based on the following sources:
The following source code was reviewed during the audit:
ecd62753bfb1f001a111ba392cfca57b02beac77
Specifically, we audited the following contracts within this repository:
Contract | SHA256 |
---|---|
contracts/crowdfund/AuctionCrowdfund.sol |
|
contracts/crowdfund/BuyCrowdfund.sol |
|
contracts/crowdfund/BuyCrowdfundBase.sol |
|
contracts/crowdfund/CollectionBuyCrowdfund.sol |
|
contracts/crowdfund/Crowdfund.sol |
|
contracts/crowdfund/CrowdfundFactory.sol |
|
contracts/crowdfund/CrowdfundNFT.sol |
|
contracts/distribution/ITokenDistributor.sol |
|
contracts/distribution/ITokenDistributorParty.sol |
|
contracts/distribution/TokenDistributor.sol |
|
contracts/gatekeepers/IGateKeeper.sol |
|
contracts/globals/Globals.sol |
|
contracts/globals/IGlobals.sol |
|
contracts/market-wrapper/IMarketWrapper.sol |
|
contracts/party/IPartyFactory.sol |
|
contracts/party/Party.sol |
|
contracts/party/PartyFactory.sol |
|
contracts/party/PartyGovernance.sol |
|
contracts/party/PartyGovernanceNFT.sol |
|
contracts/proposals/ArbitraryCallsProposal.sol |
|
contracts/proposals/FractionalizeProposal.sol |
|
contracts/proposals/IProposalExecutionEngine.sol |
|
contracts/proposals/LibProposal.sol |
|
contracts/proposals/ListOnOpenseaProposal.sol |
|
contracts/proposals/ListOnZoraProposal.sol |
|
contracts/proposals/ProposalExecutionEngine.sol |
|
contracts/proposals/ProposalStorage.sol |
|
contracts/proposals/vendor/FractionalV1.sol |
|
contracts/proposals/vendor/IOpenseaConduitController.sol |
|
contracts/proposals/vendor/IOpenseaExchange.sol |
|
contracts/tokens/ERC1155Receiver.sol |
|
contracts/tokens/ERC721Receiver.sol |
|
contracts/tokens/IERC1155.sol |
|
contracts/tokens/IERC20.sol |
|
contracts/tokens/IERC721.sol |
|
contracts/tokens/IERC721Receiver.sol |
|
contracts/utils/EIP165.sol |
|
contracts/utils/Implementation.sol |
|
contracts/utils/LibAddress.sol |
|
contracts/utils/LibERC20Compat.sol |
|
contracts/utils/LibRawResult.sol |
|
contracts/utils/LibSafeCast.sol |
|
contracts/utils/LibSafeERC721.sol |
|
contracts/utils/Proxy.sol |
|
contracts/utils/ReadOnlyDelegateCall.sol |
|
contracts/vendor/markets/IZoraAuctionHouse.sol |
|
Note: This document contains an audit solely of the Solidity contracts listed above. Specifically, the audit pertains only to the contracts themselves, and does not pertain to any other programs or scripts, including deployment scripts.
Click on an issue to jump to it, or scroll down to see them all.
lost
state at a negligible cost
getContributorInfo()
Globals
address
_burn()
function
We quantify issues in three parts:
This third part – the severity level – is a summary of how much consideration the client should give to fixing the issue. We assign severity according to the table of guidelines below:
Severity | Description |
---|---|
(C-x) Critical |
We recommend the client must fix the issue, no matter what, because not fixing would mean significant funds/assets WILL be lost. |
(H-x) High |
We recommend the client must address the issue, no matter what, because not fixing would be very bad, or some funds/assets will be lost, or the code’s behavior is against the provided spec. |
(M-x) Medium |
We recommend the client to seriously consider fixing the issue, as the implications of not fixing the issue are severe enough to impact the project significantly, albiet not in an existential manner. |
(L-x) Low |
The risk is small, unlikely, or may not relevant to the project in a meaningful way. Whether or not the project wants to develop a fix is up to the goals and needs of the project. |
(Q-x) Code Quality |
The issue identified does not pose any obvious risk, but fixing could improve overall code quality, on-chain composability, developer ergonomics, or even certain aspects of protocol design. |
(I-x) Informational |
Warnings and things to keep in mind when operating the protocol. No immediate action required. |
(G-x) Gas Optimizations |
The presented optimization suggestion would save an amount of gas significant enough, in our opinion, to be worth the development cost of implementing it. |
Party allows the ZORA auction as one proposal type in their governance contracts.
Since ZORA is custodial if you cancel the proposal while the proposal is in inprogress
, that NFT would remain inside the ZORA auction house.
Now, the party has two options: either wait for an auction to conclude, or call cancelAuction()
🔗 before the first bid is placed.
Party would need to execute an arbitrary proposal for the latter.
At this point only, the majority holder can trick the party into transferring that retrieved NFT outside of the party.
The ideal attack would look like this:
ListOnZoraProposal
with a high reserve price.Let voting happen, considering the attacker holds the majority and
there is no host intervention, and the proposal would pass.
Proposal Step 1 : createAuction()
cancelProposal()
The proposal is canceled, the party is unblocked for executing future proposals, and the NFT remains in the ZORA auction house.
Propose an ArbitraryCallsProposal
with two arbitrary calls.
Call 1: Call cancelAuction()
on the auction house contract.
Call 2: Transfer the NFT to an external address.
Let voting happen, considering the attacker holds the majority and
there is no host intervention, and the proposal would pass.
Execute Arbitrary Calls Proposal
Since the reserve price is higher compared to the market value, no one would bid.
Hence, cancelAuction()
would succeed and return the NFT to the party.
All is going as specifications expects, until here.
Now, the issue is in the second call:
Transferring NFT to an external address.
Ideally, ArbiartyCallsProposals
should fail on this call, as it is transferring precious NFT outside of the party since a check of ownership is done before and after the arbitrary calls.
bool[] memory hadPreciouses = new bool[](params.preciousTokenIds.length);
if (!isUnanimous) {
for (uint256 i = 0; i < hadPreciouses.length; ++i) {
hadPreciouses[i] = _getHasPrecious(
params.preciousTokens[i],
params.preciousTokenIds[i]
);
}
}
<
_executeSingleArbitraryCall(
i,
calls[i],
params.preciousTokens,
params.preciousTokenIds,
isUnanimous,
ethAvailable
);
>
if (!isUnanimous) {
for (uint256 i = 0; i < hadPreciouses.length; ++i) {
if (hadPreciouses[i]) {
if (!_getHasPrecious(params.preciousTokens[i], params.preciousTokenIds[i])) {
revert PreciousLostError(
params.preciousTokens[i],
params.preciousTokenIds[i]
);
}
}
}
}
However, what if the party received the NFT in one of these single arbitrary calls?
The same thing happens as in the case of cancelAuction
via arbitrary proposal.
Since the NFT is received in the middle, no record would be present in hadPreciouses
.
Hence, the attacker can move the NFT outside via a second arbitrary call.
One simple solution is to move the _getHasPrecious
check from multiple to single arbitrary calls, but that would increase gas costs for all arbitrary call proposals; another solution is to block the Zora interactions on arbitrary calls.
On line 136, CrowdfundBase.sol
has the following code inside the _buy()
function logic:
{
// Execute the call to buy the NFT.
(bool s, bytes memory r) = callTarget.call{ value: callValue }(callData);
if (!s) {
r.rawRevert();
}
}
if (token.safeOwnerOf(tokenId) == address(this)) {
if (address(this).balance >= totalContributions_) {
// If the purchase was free or the NFT was "gifted" to us,
// refund all contributors by declaring we lost.
settledPrice = 0;
expiry = 0;
emit Lost();
} else {
settledPrice = callValue;
emit Won(
// Create a party around the newly bought NFT.
party_ = _createParty(
governanceOpts,
isValidatedGovernanceOpts,
token,
tokenId
),
token,
tokenId,
callValue
);
}
There are two possible outcomes from this logic, considering that the crowdfunding contract is the owner of the target NFT (token.safeOwnerOf(tokenId) == address(this)
):
In the first scenario, the contributors will be able to burn their crowdfunding NFT to reclaim their contributions, and the gifted NFT would be locked in the contract. Given that the contract only compares the balance to the totalContributions
after the external .call()
, a malicious actor could force send ether into the crowdfunding contract, pushing the balance higher than the totalContributions
, even after the NFT is successfully purchased — essentially making the crowdfunding failed and the NFT locked.
This issue is also present in the AuctionCrowdfund.sol
contract, and, given that this contract has a receive()
function, the ether can even be sent by a regular transaction.
⚠️ In a whitelisted sale, with a crowdfunding party being exclusively allowed to buy a specific NFT or buy from a specific collection, the attack vector could be considered higher since the party will assume the purchase of the token cannot fail. And an external actor can manipulate the results.
Resolution
One consideration in mitigating this would be to check the balance before and after executing the external call and compare this difference to the total contributions:
// Verify the actual balance of the contract
uint256 balanceBefore = address(this).balance;
// Execute the call to buy the NFT.
(bool s, bytes memory r) = callTarget.call{ value: callValue }(callData);
// Make sure we acquired the NFT we want.
if (token.safeOwnerOf(tokenId) == address(this)) {
// The contract has more or the same balance than before
if (balanceBefore <= address(this).balance) {
...
emit Lost();
} else { // The contract spent eth
settledPrice = callValue;
emit Won();
}
By doing this, the contract would ignore any ether outside contributions and assure that at least some funds have been used for the purchase. Still, this is just mitigation, since the external call could be force-sending ether to the contract, but it narrows the possible attack surface
lost
state at a negligible cost
Let’s consider that there is a non-gated BuyCrowdFund
intended to buy one NFT. Multiple contributors are contributing, and all is going well.
Then, someone wants this crowdfund to fail, for some reason.
One accepted way for them to do this is to buy the given NFT. But they don't want to hold on to that NFT until expiry and don't have the funds to do so.
Another accepted way is to gift NFT to the crowdfund so that it would be locked forever, and crowdfund could be advanced to a lost
state.
But they don't want to lose their funds.
What they can do?
Well, there is one way they can directly fail this crowdfund forever: moving it to a lost
state, without losing ETH and without locking/gifting the NFT in crowdfund. They can:
Temporarily buy the NFT, and transfer it to the crowdfund contract.
Call buy()
with (callTarget=NFTContract, callData=approve(their address), callValue=”0”)
This results in the crowdfund contract approving the attacker’s address as a spender for the NFT.
Since crowdfund owns the NFT at a null cost, the contract considers that the NFT is gifted and moves to a lost
state.
Here, the assumption that NFT would remain locked forever so there is little incentive for the griefer to gift NFT won't hold true anymore
Since the NFT is still present in crowdfund and the attacker's address is the spender, they can easily move the NFT out of the contract.
Another assumption, that crowdfund would remain active until expiry if the contract doesn't own the NFT, won't hold true as well.
Transfer the NFT back to self or return it back to the marketplace by selling it on the collection floor.
ℹ️ If the attacker doesn't have initial funds to cover the buy cost, they can use the flash loan to buy NFT, given they have a buyer at the end to close the loop. If they don't have a specific buyer, they need to sell at the collection price; here, they would need to pay the difference. If they have a specific buyer — for example, some alternate party, competitor, or buyer from mempool — they can close it there.
ℹ️ If an attacker uses their own funds, they have the option to list NFT again rather than sell it. Here, though, you take risk of market volatility. So then the question is: How is it different from an attacker just buying NFT to grief crowdfund? In normal cases, the attacker needs to hold it for the whole duration to grief crowdfund. Whereas in an exploit case, you can list it soon and get it sold. So the time period over the risk that is taken is minimized.
Resolution
If NFT is considered gifted, consider directly sending it to an unreachable address like address(0)
.
This way, all previous approvals would be canceled.
Technically, locking NFT inside the crowdfund contract forever, or sending it to a 0 address, is the same thing.
Below are a few other solutions that, though possible, are not ideal:
callTarget ≠ nftContract
: not ideal since this will block the action required for NFT initial mints.While creating crowdfund, if the deployer has passed some ETH, the intention is to credit opts.initialContributor
with the initial contribution.
Crowdfund.sol L160
uint96 initialBalance = address(this).balance.safeCastUint256ToUint96(); // @audit msg.value
if (initialBalance > 0) {
// If this contract has ETH, either passed in during deployment or
// pre-existing, credit it to the `initialContributor`.
_contribute(opts.initialContributor, initialBalance, opts.initialDelegate, 0, "");
}
However, an external actor can force the code to take the path of inititalBalance > 0
, even when the deployer did not pass any ETH, by predicting the future contract address function(factory’s address, nonce)
and sending some ETH there beforehand.
By doing so, someone can grief the creation of “Crowdfunds with Zero initial contribution” as _mint
will revert by opts.initialContributor = address(0)
.
Consider checking for msg.value
instead of address(this).balance
or credit it to the msg.sender
.
We understand that this has a very lower impact and likelihood, as the deployer can always send a new transaction with intitalContributor ≠ address(0)
, but we wanted to report it because of its easy fix.
totalVotingPower
, allowing them to propose and unanimously pass proposals instantly.By doing this, any member owning an NFT with this power could bypass all the different protections for the precious NFTs by calling any proposal and having a unanimous vote because the votes are quantified with the totalVotingPower
.
totalVotingPower
can leave members without a supply of Fractionalize ERC20 vault tokens.Similar to the item above, the Fractionalize
proposal type creates a distribution of governance tokens with a supply equal to the totalVotingPower
, if there is more accumulated power than this value among the governance NFTs, possibly leaving contributors without any shares.
VETO_VALUE
voting power, disallowing the receiver to propose and vote.The vetoed value in the proposal state info it’s represented as is the type(uint96).max
value; however, if a creator of a party assigns this value as a voting power for specific governance NFT, every attempt to propose would result in the proposal having the veto value. On the other hand, if this user tries to vote for any proposal, it will revert to an arithmetic error since the proposer will accept the proposal with their power, and the max value of votes (type(uint96).max
) would be overflowed.
mint()
can overpass the uint96
max value and revert to overflow on any accept()
call.The mint function takes the votingPower
parameter as a uin256
; however, the votes are accounted for as a uint96
, the same as the above item, and if an NFT governance token has more power than the uint96
max value, the voting and proposals for that NFT would revert in every call.
The PartyDAO
specs suggest that a party could be created directly from the factory as well.
Since it's not coming from the crowdfunding route, the creator can pass anything as precious.
There is no verification if the party holds precious NFTs or not in party initialization.
At present, the creator of the party would need to transfer precious manually after the party initialization. Which they may choose not to do.
Consider transferring NFTs inside the party initialization to get rid of this trust assumption. We couldn't think of any impact this would have, except wrong information being given to the user and some added restrictions in arbitrary calls proposal — which is why we marked this as quality.
In the Globals contract, consider making transferMultiSig() a two-step process, where the new multi-sig needs to accept the ownership to have an effect. This would protect from incorrect updates done on highly powerful functions.
The party contracts assume that the underlying NFTs are following the ERC721 standard.
If any underlying NFTs are not following standard, the code could break at various points.
While doing _setPreciousList
in party initialization, consider adding the supportsInterface
check on passed NFTs.
The Governance contracts have a cancel()
function to prevent having a locked status on the party when a proposal has been in progress for an undesired amount of time. Although this function call can “unlock” the contract, it can also leave it in an unrecoverable state, even losing a precious asset.
To ensure that a cautious time is set to this inside the proposals, consider making the minimum cap for the cancelDelay
time the currently present cap for the maximum
cancel delay.
In the PartyGovernance, the voting power used for the accept()
function can be zero, and this can cause unnecessary gas spent if a user mistakenly calls the accept function without any voting power.
If the likelihood of users calling accept even when they don't have voting power is high, consider adding a check on top.
getContributorInfo()
For getContributorInfo()
of CrowdFund.sol
, there is no need to calculate ethContributed
by looping over all contributions for won and lost states; the same result could be achieved by adding ethOwed
and ethUsed
.
In the contract's code, there are a few inconsistencies with the spec and code logic in the comments regarding the voting power.
// Caller must own a governance NFT at the current time.
modifier onlyActiveMember() {
...
}
// Caller must own a governance NFT at the current time or be the `Party` instance.
modifier onlyActiveMemberOrSelf() {
...
}
PartyGovernance.sol
, line 545:/// @dev Only an active member (owns a governance token) can call this.
/// Afterwards, members can vote to support it with accept() or a party
/// host can unilaterally reject the proposal with veto().
PartyGovernance.sol
, line 578:/// @dev The voting power cast will be the effective voting power of the caller
/// at the time propose() was called (see `getVotingPowerAt()`).
/// If the proposal reaches `passThresholdBps` acceptance ratio then the
/// proposal will be in the `Passed` state and will be executable after
/// the `executionDelay` has passed, putting it in the `Ready` state.
When determining the effective voting power of a user, we binary search a user's voting power records for the most recent record <= the proposal time.
It should be <
instead of <=
.
Consider renaming onlyHostCanBuy to a more appropriate name. The naming suggests that only the host can call buy; however, if set to true and there are gatekeeper addresses set, the code also allows gated contributors to buy.
Globals
address
The Globals
address variable is present in both the CrowdFund
and CrowdFundNFT
contracts as private immutable.
CrowdFund inherits from CrowdFundNFT, hence the same result could be achieved by having Globals
as public or internal immutable inside the CrowdFundNFT
contract only.
_burn()
function
In the Crowdfund.sol contract, there is no need for the check at line 548 of the _burn() function. If Crowdfund state is won, its given party address will not be zero for all types of crowdfunding.
On line 105, BuyCrowdfundBase
has the following check:
// Ensure the call target isn't trying to reenter
if (callTarget == address(this)) {
revert InvalidCallTargetError(callTarget);
}
Although the callTarget
will revert if someone tries to re-enter the contract, another smart contract could be the intermediary and achieve a re-entrant call, though it would be useless since there is another re-entrancy protection on line 127:
// Temporarily set to non-zero as a reentrancy guard.
settledPrice = type(uint96).max;
By setting the settledPrice
to a value different from 0
, the function buy()
is already guarded against any re-entrant call attempts because the function getCrowdfundLifecycle
will make the execution revert to a CrowdfundLifecycle.Busy
state.
The same thing is present in line 161, inside the ArbitraryCallsProposal
contract:
// Cannot call ourselves.
if (call.target == address(this)) {
return false;
}
And it is mitigated with a similar approach in the ProposalExecutionEngine
contract, in line 162.
Macro makes no warranties, either express, implied, statutory, or otherwise, with respect to the services or deliverables provided in this report, and Macro specifically disclaims all implied warranties of merchantability, fitness for a particular purpose, noninfringement and those arising from a course of dealing, usage or trade with respect thereto, and all such warranties are hereby excluded to the fullest extent permitted by law.
Macro will not be liable for any lost profits, business, contracts, revenue, goodwill, production, anticipated savings, loss of data, or costs of procurement of substitute goods or services or for any claim or demand by any other party. In no event will Macro be liable for consequential, incidental, special, indirect, or exemplary damages arising out of this agreement or any work statement, however caused and (to the fullest extent permitted by law) under any theory of liability (including negligence), even if Macro has been advised of the possibility of such damages.
The scope of this report and review is limited to a review of only the code presented by the PartyDAO team and only the source code Macro notes as being within the scope of Macro’s review within this report. This report does not include an audit of the deployment scripts used to deploy the Solidity contracts in the repository corresponding to this audit. Specifically, for the avoidance of doubt, this report does not constitute investment advice, is not intended to be relied upon as investment advice, is not an endorsement of this project or team, and it is not a guarantee as to the absolute security of the project. In this report you may through hypertext or other computer links, gain access to websites operated by persons other than Macro. Such hyperlinks are provided for your reference and convenience only, and are the exclusive responsibility of such websites’ owners. You agree that Macro is not responsible for the content or operation of such websites, and that Macro shall have no liability to your or any other person or entity for the use of third party websites. Macro assumes no responsibility for the use of third party software and shall have no liability whatsoever to any person or entity for the accuracy or completeness of any outcome generated by such software.