Platform: Code4rena
Start Date: 12/09/2022
Pot Size: $75,000 USDC
Total HM: 19
Participants: 110
Period: 7 days
Judge: HardlyDifficult
Total Solo HM: 9
Id: 160
League: ETH
Rank: 9/110
Findings: 3
Award: $1,858.14
🌟 Selected for report: 0
🚀 Solo Findings: 0
1739.5676 USDC - $1,739.57
https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ArbitraryCallsProposal.sol#L112 https://github.com/PartyDAO/party-contracts-c4/blob/3896577b8f0fa16cba129dc2867aba786b730c1b/contracts/proposals/ArbitraryCallsProposal.sol#L104
Detailed description of the impact of this finding.
Let's look into the implementation in ArbitraryCallsProposal.sol
// Check that the call is not prohibited. if (!_isCallAllowed(call, isUnanimous, preciousTokens, preciousTokenIds)) { revert CallProhibitedError(call.target, call.data); } // Check that we have enough ETH to execute the call. if (ethAvailable < call.value) { revert NotEnoughEthAttachedError(call.value, ethAvailable); } // Execute the call. (bool s, bytes memory r) = call.target.call{ value: call.value }(call.data);
the _isCallAllowed make sure that malicious actor cannot approve a malicious address spending party's nft for ERC721 token.
if (selector == IERC721.approve.selector) { // Can only call `approve()` on the precious if the operator is null. (address op, uint256 tokenId) = _decodeApproveCallDataArgs(call.data); if (op != address(0)) { return !LibProposal.isTokenIdPrecious( IERC721(call.target), tokenId, preciousTokens, preciousTokenIds ); } // Can only call `setApprovalForAll()` on the precious if // toggling off. } else if (selector == IERC721.setApprovalForAll.selector) { (, bool isApproved) = _decodeSetApprovalForAllCallDataArgs(call.data); if (isApproved) { return !LibProposal.isTokenPrecious(IERC721(call.target), preciousTokens); } }
note the if condition
if (selector == IERC721.approve.selector)
but the malicious member can create a proposal to approve a malicious contract to spend ERC1155 NFT and ERC20 tokens.
Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.
a ERC1155 NFT is purchased and party is created.
a malicious member create a proposal to approve a malicious smart contract to spend the ERC1155 token in the party contract via the ArbitraryCallsProposal.sol
// Execute the call. (bool s, bytes memory r) = call.target.call{ value: call.value }(call.data);
the call.target would be ERC1155 nft contract, the payload would be the setApprovalFor function signature + the malicious smart contract call data.
a ERC721 NFT is purchase and party is created.
a FractionalizedProposal is passed and the NFT is fractionalized into N supply of ERC20 token share.
A malicious member create a proposal to approve malicious spending for a malicious contract to spend the ERC20 token in the party contract
by calling
// Execute the call. (bool s, bytes memory r) = call.target.call{ value: call.value }(call.data);
the target would be the ERC20 address, the call data and payload is the ERC20 token approve function signature + the malicious smart contract address call data.
Manual Review Foundry
We recommend the project check if the party still holds the NFT in the party contract after each call
We recommend the project disable the ERC20 token approval.
#0 - merklejerk
2022-09-21T21:26:01Z
#1 - trust1995
2022-10-06T14:06:29Z
Dupped the wrong submission, the correct one is #277
#2 - trust1995
2022-10-06T14:35:04Z
@HardlyDifficult I mistakenely dupped it to #264 before, just making sure the bookkeeping is correct.
#3 - HardlyDifficult
2022-10-06T14:37:09Z
@HardlyDifficult I mistakenely dupped it to #264 before, just making sure the bookkeeping is correct.
thanks for confirming. i had looked at your previous comment but didn't understand the connection so I set it aside till later 😆 so no problems! thanks for the inputs
🌟 Selected for report: Lambda
Also found by: 0x1f8b, 0x4non, 0x52, 0x5rings, 0xDanielC, 0xNazgul, 0xSmartContract, 0xbepresent, Anth3m, Aymen0909, B2, CRYP70, CertoraInc, Ch_301, Chom, ChristianKuri, CodingNameKiki, Deivitto, Funen, JC, JansenC, Jeiwan, KIntern_NA, MasterCookie, MiloTruck, Olivierdem, PaludoX0, R2, RaymondFam, ReyAdmirado, StevenL, The_GUILD, Tomo, Trust, V_B, __141345__, asutorufos, ayeslick, bin2chen, brgltd, bulej93, c3phas, cccz, ch0bu, cryptphi, csanuragjain, d3e4, delfin454000, djxploit, erictee, fatherOfBlocks, gogo, hansfriese, indijanc, ladboy233, leosathya, lukris02, malinariy, martin, pedr02b2, pfapostol, rvierdiiev, slowmoses, smiling_heretic, tnevler, wagmi
83.2171 USDC - $83.22
// keccak256(abi.encodeWithSignature('Error(string)', "Auction doesn't exit")) bytes32 constant internal AUCTION_DOESNT_EXIST_ERROR_HASH = 0x474ba0184a7cd5de777156a56f3859150719340a6974b6ee50f05c58139f4dc2;
should be
// keccak256(abi.encodeWithSignature('Error(string)', "Auction doesn't exist"))
The project requires the integration of Zora, Opensea and fractionalize protocol. We recommand project add unit test and integration test to increase the test coverage for the contract below
name test coverage contracts/crowdfund/CrowdfundNFT.sol 44.00% contracts/party/PartyGovernanceNFT.sol 57.69% contracts/party/PartyFactory.sol 🌀 80.00% contracts/proposals/ProposalStorage.sol 🖥 👥 🧮 80.00% contracts/proposals/ProposalExecutionEngine.sol 🖥 86.44%
Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.
https://swcregistry.io/docs/SWC-103
all the codes are using
// SPDX-License-Identifier: Beta Software pragma solidity ^0.8;
we recommend locking the solidity version.
Use a solidity version of at least 0.8.2 to get simple compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value
all the codes are using
// SPDX-License-Identifier: Beta Software pragma solidity ^0.8;
we recommend using up to date solidity version
function _burn(address payable contributor, CrowdfundLifecycle lc, Party party_) private {
the function does not check if the contributor is a address(0)
burn(address(0)) alaways succeed.
we recommend the project handle the external call to make sure the external call do not fail silently.
preciousTokens[i].transferFrom(address(this), address(party_), preciousTokenIds[i]);
(success, ) = targetAddress.call{value: amountEth}(targetCallData);
We recommend the project add emit emission when changing important state
function disableEmergencyActions() onlyPartyDao external { emergencyActionsDisabled = true; }
function disableEmergencyExecute() external onlyPartyDaoOrHost onlyDelegateCall { emergencyExecuteDisabled = true; }
#0 - 0xble
2022-09-26T01:56:37Z
Unhelpful bot report with some suggestions that don't make sense.
#1 - HardlyDifficult
2022-09-30T21:24:52Z
🌟 Selected for report: CertoraInc
Also found by: 0x1f8b, 0x4non, 0x5rings, 0x85102, 0xNazgul, 0xSmartContract, 0xkatana, Amithuddar, Aymen0909, B2, Bnke0x0, CRYP70, Chom, ChristianKuri, CodingNameKiki, Deivitto, Diraco, Fitraldys, Funen, IgnacioB, JAGADESH, JC, Lambda, LeoS, Matin, Metatron, MiloTruck, Noah3o6, Ocean_Sky, Olivierdem, PaludoX0, RaymondFam, ReyAdmirado, Rohan16, Rolezn, Saintcode_, Sm4rty, SnowMan, StevenL, Tomio, Tomo, V_B, Waze, __141345__, ajtra, asutorufos, aysha, brgltd, bulej93, c3phas, ch0bu, d3e4, delfin454000, dharma09, djxploit, erictee, fatherOfBlocks, francoHacker, gianganhnguyen, gogo, got_targ, ignacio, jag, karanctf, ladboy233, leosathya, lukris02, m_Rassska, malinariy, martin, natzuu, pashov, peanuts, peiw, pfapostol, prasantgupta52, robee, simon135, slowmoses, sryysryy, tnevler
35.348 USDC - $35.35
return abi.encode(ListOnOpenseaStep.ListedOnZora, ZoraProgressData({
return abi.encode(ListOnOpenseaStep.ListedOnOpenSea, orderHash, expiry);
return abi.encode(ZoraStep.ListedOnZora, ZoraProgressData({
The overheads outlined below are PER LOOP, excluding the first loop
storage arrays incur a Gwarmaccess (100 gas) memory arrays use MLOAD (3 gas) calldata arrays use CALLDATALOAD (3 gas) Caching the length changes each of these to a DUP<N> (3 gas), and gets rid of the extra DUP<N> needed to store the stack offset
for (uint256 i=0; i < opts.hosts.length; ++i) {
for (uint256 i = 0; i < infos.length; ++i) {
for (uint256 i = 0; i < infos.length; ++i) {
for (uint256 i = 0; i < hadPreciouses.length; ++i) {
for (uint256 i = 0; i < calls.length; ++i) {
for (uint256 i = 0; i < hadPreciouses.length; ++i) {
for (uint256 i = 0; i < preciousTokens.length; ++i) {
for (uint256 i = 0; i < preciousTokens.length; ++i) {
for (uint256 i = 0; i < fees.length; ++i) {
for (uint256 i; i < hosts.length; i++) {
for (uint256 i = 0; i < contributors.length; ++i) {
for (uint256 i = 0; i < preciousTokens.length; ++i) {
for (uint256 i=0; i < opts.hosts.length; ++i) {
for (uint256 i = 0; i < infos.length; ++i) {
for (uint256 i = 0; i < infos.length; ++i) {
for (uint256 i = 0; i < hadPreciouses.length; ++i) {
for (uint256 i = 0; i < calls.length; ++i) {
for (uint256 i = 0; i < hadPreciouses.length; ++i) {
for (uint256 i = 0; i < preciousTokens.length; ++i) {
for (uint256 i = 0; i < preciousTokens.length; ++i) {
for (uint256 i = 0; i < fees.length; ++i) {
for (uint256 i = 0; i < contributors.length; ++i) {
for (uint256 i = 0; i < numContributions; ++i) {
for (uint256 i = 0; i < preciousTokens.length; ++i) {
for (uint256 i = 0; i < numContributions; ++i) {
Saves 6 gas per loop
for (uint256 i; i < hosts.length; i++) {
function contribute(address delegate, bytes memory gateData)
Can use memory instead of storage.
Contribution[] storage contributions = _contributionsByContributor[contributor];
Contribution[] storage contributions = _contributionsByContributor[contributor];