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: 21/110
Findings: 2
Award: $262.74
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 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
88.3642 USDC - $88.36
In general, the codebase is well-written but looks a bit overcomplicated (usage of assembler, many structures with the same fields, custom implementation of common patterns). Contracts can be contributed using "standard" libraries like solmate or openzeppilin instead of a custom implementation of common patterns (eg. reentrancy protection). Or, if a custom implementation is preferred, they should be better documented.
| Issue | Instances | |
|---|---|---|
| 1 | require()/revert() statements should have descriptive reason strings/error | 4 |
| 2 | Critical changes should use two-step procedure | 1 |
| 3 | Missing checks for address(0x0) when assigning values to address immutable variables or RBAC variables | 6 |
| 4 | Unspecific Compiler Version Pragma | 26 |
Low-Risk Issues:
An empty revert string doesn't give the user any information about the reason for the revert, so they can't fix it and use contracts, or they need to lose some gas to try and figure it out by trial and error.
diff --git a/scope/ProposalExecutionEngine.sol b/scope/ProposalExecutionEngine.sol index 7961b28..ac3d19b 100644 --- a/scope/ProposalExecutionEngine.sol +++ b/scope/ProposalExecutionEngine.sol @@ -243,8 +243,8 @@ contract ProposalExecutionEngine is 243, 243: mstore(add(proposalData, 4), sub(mload(proposalData), 4)) 244, 244: offsetProposalData := add(proposalData, 4) 245, 245: } - 246 :- require(proposalType != ProposalType.Invalid); - 247 :- require(uint8(proposalType) < uint8(ProposalType.NumProposalTypes)); + 246:+ require(proposalType != ProposalType.Invalid, "Invalid proposal type"); + 247:+ require(uint8(proposalType) < uint8(ProposalType.NumProposalTypes), "Invalid proposal type"); 248, 248: } 249, 249: 250, 250: // Upgrade implementation to the latest version.
diff --git a/scope/ReadOnlyDelegateCall.sol b/scope/ReadOnlyDelegateCall.sol index c2e6313..bc99cd1 100644 --- a/scope/ReadOnlyDelegateCall.sol +++ b/scope/ReadOnlyDelegateCall.sol @@ -17,7 +17,7 @@ contract ReadOnlyDelegateCall { 17, 17: // Delegatecall into implement and revert with the raw result. 18, 18: function delegateCallAndRevert(address impl, bytes memory callData) external { 19, 19: // Attempt to gate to only `_readOnlyDelegateCall()` invocations. - 20 :- require(msg.sender == address(this)); + 20:+ require(msg.sender == address(this), "Only Delegate Call"); 21, 21: (bool s, bytes memory r) = impl.delegatecall(callData); 22, 22: // Revert with success status and return data. 23, 23: abi.encode(s, r).rawRevert();
Any errors/typos in the address can lead to access blocking
diff --git a/contracts/globals/Globals.sol b/contracts/globals/Globals.sol index 71b1f89..487a869 100644 --- a/contracts/globals/Globals.sol +++ b/contracts/globals/Globals.sol @@ -6,12 +6,15 @@ import "./IGlobals.sol"; 6, 6: /// @notice Contract storing global configuration values. 7, 7: contract Globals is IGlobals { 8, 8: address public multiSig; + 9:+ + 10:+ address private pendingMultiSig; 9, 11: // key -> word value 10, 12: mapping(uint256 => bytes32) private _wordValues; 11, 13: // key -> word value -> isIncluded 12, 14: mapping(uint256 => mapping(bytes32 => bool)) private _includedWordValues; 13, 15: 14, 16: error OnlyMultiSigError(); + 17:+ error OnlyPendingMultiSig(); 15, 18: 16, 19: modifier onlyMultisig() { 17, 20: if (msg.sender != multiSig) { @@ -25,7 +28,13 @@ contract Globals is IGlobals { 25, 28: } 26, 29: 27, 30: function transferMultiSig(address newMultiSig) external onlyMultisig { - 28 :- multiSig = newMultiSig; + 31:+ pendingMultiSig = newMultiSig; + 32:+ } + 33:+ + 34:+ function acceptMultiSig() external { + 35:+ if (msg.sender != pendingMultiSig) revert OnlyPendingMultiSig(); + 36:+ multiSig = pendingMultiSig; + 37:+ delete pendingMultiSig; 29, 38: } 30, 39: 31, 40: function getBytes32(uint256 key) external view returns (bytes32) {
Loss of access
diff --git a/contracts/globals/Globals.sol b/contracts/globals/Globals.sol index 71b1f89..d08499b 100644 --- a/contracts/globals/Globals.sol +++ b/contracts/globals/Globals.sol @@ -12,6 +12,7 @@ contract Globals is IGlobals { 12, 12: mapping(uint256 => mapping(bytes32 => bool)) private _includedWordValues; 13, 13: 14, 14: error OnlyMultiSigError(); + 15:+ error ZeroAddress(); 15, 16: 16, 17: modifier onlyMultisig() { 17, 18: if (msg.sender != multiSig) { @@ -21,10 +22,12 @@ contract Globals is IGlobals { 21, 22: } 22, 23: 23, 24: constructor(address multiSig_) { + 25:+ if (multiSig_ == address(0)) revert ZeroAddress(); 24, 26: multiSig = multiSig_; 25, 27: } 26, 28: 27, 29: function transferMultiSig(address newMultiSig) external onlyMultisig { + 30:+ if (newMultiSig == address(0)) revert ZeroAddress(); 28, 31: multiSig = newMultiSig; 29, 32: } 30, 33:
The implementation logic requires that the contract be created with a constructor and assigned to global objects before initialization, but it is possible to call initialization via factories on an uncreated contract (Implementation(address(0))) and this call can succeed, but any other function will revert.
I think it's better to check in the getter because the setter can use the null address to remove the implementation.
diff --git a/contracts/globals/Globals.sol b/contracts/globals/Globals.sol index d08499b..6c917f0 100644 --- a/contracts/globals/Globals.sol +++ b/contracts/globals/Globals.sol @@ -32,19 +32,27 @@ contract Globals is IGlobals { 32, 32: } 33, 33: 34, 34: function getBytes32(uint256 key) external view returns (bytes32) { - 35 :- return _wordValues[key]; + 35:+ bytes32 value_; + 36:+ if ((value_ = _wordValues[key]) == bytes32(0)) revert ZeroAddress(); + 37:+ return value_; 36, 38: } 37, 39: 38, 40: function getUint256(uint256 key) external view returns (uint256) { - 39 :- return uint256(_wordValues[key]); + 41:+ bytes32 value_; + 42:+ if ((value_ = _wordValues[key]) == bytes32(0)) revert ZeroAddress(); + 43:+ return uint256(value_); 40, 44: } 41, 45: 42, 46: function getAddress(uint256 key) external view returns (address) { - 43 :- return address(uint160(uint256(_wordValues[key]))); + 47:+ bytes32 value_; + 48:+ if ((value_ = _wordValues[key]) == bytes32(0)) revert ZeroAddress(); + 49:+ return address(uint160(uint256(value_))); 44, 50: } 45, 51: 46, 52: function getImplementation(uint256 key) external view returns (Implementation) { - 47 :- return Implementation(address(uint160(uint256(_wordValues[key])))); + 53:+ bytes32 value_; + 54:+ if ((value_ = _wordValues[key]) == bytes32(0)) revert ZeroAddress(); + 55:+ return Implementation(address(uint160(uint256(value_)))); 48, 56: } 49, 57: 50, 58: function getIncludesBytes32(uint256 key, bytes32 value) external view returns (bool) {
contract ZeroImplPoC is Test { Globals globals = new Globals(address(this)); // @note the party variable exists, but the constructor has not been run Party party; AuctionCrowdfund auctionCrowdfund = new AuctionCrowdfund(globals); CrowdfundFactory crowdfundFactory = new CrowdfundFactory(globals); AuctionCrowdfund.AuctionCrowdfundOptions auctionCrowdfundOptions; Crowdfund.FixedGovernanceOpts fixedGovernanceOpts; PartyFactory partyFactory = new PartyFactory(globals); MockMarketWrapper market = new MockMarketWrapper(); DummyERC721 tokenToBuy; IGateKeeper defaultGateKeeper; bytes12 defaultGateKeeperId; ProposalExecutionEngine eng = new ProposalExecutionEngine( globals, IOpenseaExchange(vm.addr(0x053)), IOpenseaConduitController(vm.addr(0x0533)), IZoraAuctionHouse(vm.addr(0x507aa4)), IFractionalV1VaultFactory(vm.addr(0xf14f)) ); address initialContributor = vm.addr(0x1417141); address initialDelegate = vm.addr(0xd417141); address host1 = vm.addr(0x40571); address host2 = vm.addr(0x40572); address host3 = vm.addr(0x40573); address contributor1 = vm.addr(0xc04761b1); address delegate1 = vm.addr(0xd04761b1); address bidder = vm.addr(0xb1d37); address splitRecipient = vm.addr(0x5117); uint256 auctionId; uint256 tokenId; function setup_options() internal { fixedGovernanceOpts.hosts.push(host1); fixedGovernanceOpts.hosts.push(host2); fixedGovernanceOpts.hosts.push(host3); fixedGovernanceOpts.voteDuration = 1 days; fixedGovernanceOpts.executionDelay = 0.5 days; fixedGovernanceOpts.passThresholdBps = 0.51e4; (auctionId, tokenId) = market.createAuction(1337); tokenToBuy = market.nftContract(); auctionCrowdfundOptions = AuctionCrowdfund.AuctionCrowdfundOptions({ name: "AuditCrowdfund", symbol: "AC", auctionId: auctionId, market: market, nftContract: tokenToBuy, nftTokenId: tokenId, duration: 1 hours, maximumBid: 10e18, splitRecipient: payable(splitRecipient), splitBps: 1e3, initialContributor: initialContributor, initialDelegate: initialDelegate, gateKeeper: defaultGateKeeper, gateKeeperId: defaultGateKeeperId, governanceOpts: fixedGovernanceOpts }); } function setup_globals() internal { globals.setAddress( LibGlobals.GLOBAL_AUCTION_CF_IMPL, address(auctionCrowdfund) ); globals.setAddress( LibGlobals.GLOBAL_PARTY_FACTORY, address(partyFactory) ); globals.setAddress( LibGlobals.GLOBAL_PROPOSAL_ENGINE_IMPL, address(eng) ); globals.setAddress(LibGlobals.GLOBAL_PARTY_IMPL, address(party)); } function test_zero_impl() public { setup_options(); setup_globals(); vm.deal(initialContributor, 1 ether); vm.prank(initialContributor); auctionCrowdfund = crowdfundFactory.createAuctionCrowdfund{value: 100}( auctionCrowdfundOptions, "" ); vm.deal(contributor1, 1 ether); vm.prank(contributor1); auctionCrowdfund.contribute{value: 1337}(delegate1, ""); vm.prank(bidder); auctionCrowdfund.bid(); market.endAuction(auctionId); // @note will not revert. party = auctionCrowdfund.finalize(fixedGovernanceOpts); auctionCrowdfund.burn(payable(contributor1)); auctionCrowdfund.burn(payable(initialContributor)); console.logAddress(globals.getAddress(LibGlobals.GLOBAL_PARTY_IMPL)); // @note will revert party.getProposalExecutionEngine(); //revert without reasons // @note will revert party.getVotingPowerAt(initialDelegate, uint40(block.timestamp)); // @note will revert PartyGovernance.Proposal memory proposal = PartyGovernance.Proposal({ maxExecutableTime: uint40(block.timestamp + 1000), cancelDelay: 100, proposalData: abi.encodePacked([0]) }); vm.prank(initialDelegate); uint256 proposalId = party.propose(proposal, 13); } }
Avoid floating pragmas for non-library contracts.
While floating pragmas make sense for libraries to allow them to be included with multiple different versions of applications, it may be a security risk for application implementations.
A known vulnerable compiler version may accidentally be selected or security tools might fall-back to an older compiler version ending up checking a different EVM compilation that is ultimately deployed on the blockchain.
It is recommended to pin to a concrete compiler version.
Avoid floating pragmas. We highly recommend pinning a concrete compiler version (latest without security issues) in at least the top-level “deployed” contracts to make it unambiguous which compiler version is being used. Rule of thumb: a flattened source-unit should have at least one non-floating concrete solidity compiler version pragma.
ArbitraryCallsProposal.sol:pragma solidity ^0.8; AuctionCrowdfund.sol:pragma solidity ^0.8; BuyCrowdfundBase.sol:pragma solidity ^0.8; BuyCrowdfund.sol:pragma solidity ^0.8; CollectionBuyCrowdfund.sol:pragma solidity ^0.8; CrowdfundFactory.sol:pragma solidity ^0.8; CrowdfundNFT.sol:pragma solidity ^0.8; Crowdfund.sol:pragma solidity ^0.8; EIP165.sol:pragma solidity ^0.8; ERC1155Receiver.sol:pragma solidity ^0.8; ERC721Receiver.sol:pragma solidity ^0.8; FractionalizeProposal.sol:pragma solidity ^0.8; FractionalV1.sol:pragma solidity ^0.8; Globals.sol:pragma solidity ^0.8; Implementation.sol:pragma solidity ^0.8; ListOnOpenseaProposal.sol:pragma solidity ^0.8; ListOnZoraProposal.sol:pragma solidity ^0.8; PartyFactory.sol:pragma solidity ^0.8; PartyGovernanceNFT.sol:pragma solidity ^0.8; PartyGovernance.sol:pragma solidity ^0.8; Party.sol:pragma solidity ^0.8; ProposalExecutionEngine.sol:pragma solidity ^0.8; ProposalStorage.sol:pragma solidity ^0.8; Proxy.sol:pragma solidity ^0.8; ReadOnlyDelegateCall.sol:pragma solidity ^0.8; TokenDistributor.sol:pragma solidity ^0.8;
🌟 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
174.3822 USDC - $174.38
Gas savings are estimated using the gas report of existing yarn test:gas tests (the sum of all deployment costs and the sum of the costs of calling all methods + forge snapshot --diff) and may vary depending on the implementation of the fix.
NOTE: with current tests, method call evaluation is very volatile (±300 000 for the same code), so I include the results of forge snapshot --diff because they are more stable.
| Issue | Instances | Estimated gas(deployments) | Estimated gas(avg method call) | forge snapshot --diff | |
|---|---|---|---|---|---|
| 1 | Expression can be unchecked when overflow is not possible | 16 | 58 069 | 11 431 ± 300 000 | 708 160 |
| 2 | Using bools for storage incurs overhead | 6 | 35 242 | 137 433 ± 300 000 | 678 523 |
| 3 | State variables should be cached in stack variables rather than re-reading them from storage | 5 | 21 616 | 45 963 ± 300 000 | 116 899 |
| 4 | Use custom errors rather than revert()/require() strings to save gas | 1 | 18 424 | -3 095 ± 300 000 | 126 535 |
| 5 | <array>.length should not be looked up in every loop of a for-loop | 3 | 8 200 | 62 475 ± 300 000 | 17 105 |
| 6 | Division by two should use bit shifting | 1 | 1 600 | 222 938 ± 300 000 | 87 318 |
| 7 | Call functions only when you need them | 1 | 600 | -33567 ± 300 000 | 616 |
| 8 | Prefix increments are cheaper than postfix increments, especially when it's used in for-loops | 1 | 400 | -49907 ± 300 000 | 5 |
| Overall Gas Saved | 34 | 141 372 | 3590 ± 300 000 | 1 698 279 |
Total: 34 instances over 8 issues
Deployment. Gas Saved: 58 069
Minumal Method Call. Gas Saved: -104 ± 300 000
Average Method Call. Gas Saved: 11 431 ± 300 000
Maximum Method Call. Gas Saved: 202 526 ± 300 000
forge snapshot --diff. Gas Saved: 708 160
diff --git a/contracts/crowdfund/Crowdfund.sol b/contracts/crowdfund/Crowdfund.sol index 5c27346..3cd3013 100644 --- a/contracts/crowdfund/Crowdfund.sol +++ b/contracts/crowdfund/Crowdfund.sol @@ -177,8 +177,11 @@ abstract contract Crowdfund is ERC721Receiver, CrowdfundNFT { 177, 177: { 178, 178: Party party_ = party; 179, 179: CrowdfundLifecycle lc = getCrowdfundLifecycle(); - 180 :- for (uint256 i = 0; i < contributors.length; ++i) { + 180:+ for (uint256 i = 0; i < contributors.length;) { 181, 181: _burn(contributors[i], lc, party_); + 182:+ unchecked { + 183:+ ++i; + 184:+ } 182, 185: } 183, 186: } 184, 187: @@ -239,8 +242,11 @@ abstract contract Crowdfund is ERC721Receiver, CrowdfundNFT { 239, 242: CrowdfundLifecycle lc = getCrowdfundLifecycle(); 240, 243: Contribution[] storage contributions = _contributionsByContributor[contributor]; 241, 244: uint256 numContributions = contributions.length; - 242 :- for (uint256 i = 0; i < numContributions; ++i) { + 245:+ for (uint256 i = 0; i < numContributions;) { 243, 246: ethContributed += contributions[i].amount; + 247:+ unchecked { + 248:+ ++i; + 249:+ } 244, 250: } 245, 251: if (lc == CrowdfundLifecycle.Won || lc == CrowdfundLifecycle.Lost) { 246, 252: (ethUsed, ethOwed, votingPower) = _getFinalContribution(contributor); @@ -297,8 +303,11 @@ abstract contract Crowdfund is ERC721Receiver, CrowdfundNFT { 297, 303: preciousTokenIds 298, 304: ); 299, 305: // Transfer the acquired NFTs to the new party. - 300 :- for (uint256 i = 0; i < preciousTokens.length; ++i) { + 306:+ for (uint256 i = 0; i < preciousTokens.length;) { 301, 307: preciousTokens[i].transferFrom(address(this), address(party_), preciousTokenIds[i]); + 308:+ unchecked { + 309:+ ++i; + 310:+ } 302, 311: } 303, 312: } 304, 313: @@ -345,7 +354,7 @@ abstract contract Crowdfund is ERC721Receiver, CrowdfundNFT { 345, 354: { 346, 355: Contribution[] storage contributions = _contributionsByContributor[contributor]; 347, 356: uint256 numContributions = contributions.length; - 348 :- for (uint256 i = 0; i < numContributions; ++i) { + 357:+ for (uint256 i = 0; i < numContributions;) { 349, 358: Contribution memory c = contributions[i]; 350, 359: if (c.previousTotalContributions >= totalEthUsed) { 351, 360: // This entire contribution was not used. @@ -359,6 +368,9 @@ abstract contract Crowdfund is ERC721Receiver, CrowdfundNFT { 359, 368: ethUsed += partialEthUsed; 360, 369: ethOwed = c.amount - partialEthUsed; 361, 370: } + 371:+ unchecked { + 372:+ ++i; + 373:+ } 362, 374: } 363, 375: } 364, 376: // one SLOAD with optimizer on
diff --git a/contracts/proposals/ArbitraryCallsProposal.sol b/contracts/proposals/ArbitraryCallsProposal.sol index ec164bc..8afd4d2 100644 --- a/contracts/proposals/ArbitraryCallsProposal.sol +++ b/contracts/proposals/ArbitraryCallsProposal.sol @@ -49,16 +49,19 @@ contract ArbitraryCallsProposal { 49, 49: // so we can check that we still have them later. 50, 50: bool[] memory hadPreciouses = new bool[](params.preciousTokenIds.length); 51, 51: if (!isUnanimous) { - 52 :- for (uint256 i = 0; i < hadPreciouses.length; ++i) { + 52:+ for (uint256 i = 0; i < hadPreciouses.length;) { 53, 53: hadPreciouses[i] = _getHasPrecious( 54, 54: params.preciousTokens[i], 55, 55: params.preciousTokenIds[i] 56, 56: ); + 57:+ unchecked { + 58:+ ++i; + 59:+ } 57, 60: } 58, 61: } 59, 62: // Can only forward ETH attached to the call. 60, 63: uint256 ethAvailable = msg.value; - 61 :- for (uint256 i = 0; i < calls.length; ++i) { + 64:+ for (uint256 i = 0; i < calls.length;) { 62, 65: // Execute an arbitrary call. 63, 66: _executeSingleArbitraryCall( 64, 67: i, @@ -71,11 +74,14 @@ contract ArbitraryCallsProposal { 71, 74: // Update the amount of ETH available for the subsequent calls. 72, 75: ethAvailable -= calls[i].value; 73, 76: emit ArbitraryCallExecuted(params.proposalId, i, calls.length); + 77:+ unchecked { + 78:+ ++i; + 79:+ } 74, 80: } 75, 81: // If not a unanimous vote and we had a precious beforehand, 76, 82: // ensure that we still have it now. 77, 83: if (!isUnanimous) { - 78 :- for (uint256 i = 0; i < hadPreciouses.length; ++i) { + 84:+ for (uint256 i = 0; i < hadPreciouses.length;) { 79, 85: if (hadPreciouses[i]) { 80, 86: if (!_getHasPrecious(params.preciousTokens[i], params.preciousTokenIds[i])) { 81, 87: revert PreciousLostError( @@ -84,6 +90,9 @@ contract ArbitraryCallsProposal { 84, 90: ); 85, 91: } 86, 92: } + 93:+ unchecked { + 94:+ ++i; + 95:+ } 87, 96: } 88, 97: } 89, 98: // No next step, so no progressData.
diff --git a/contracts/crowdfund/CollectionBuyCrowdfund.sol b/contracts/crowdfund/CollectionBuyCrowdfund.sol index 73b7e66..c7cc5ea 100644 --- a/contracts/crowdfund/CollectionBuyCrowdfund.sol +++ b/contracts/crowdfund/CollectionBuyCrowdfund.sol @@ -59,11 +59,14 @@ contract CollectionBuyCrowdfund is BuyCrowdfundBase { 59, 59: 60, 60: modifier onlyHost(address[] memory hosts) { 61, 61: bool isHost; - 62 :- for (uint256 i; i < hosts.length; ++i) { + 62:+ for (uint256 i; i < hosts.length;) { 63, 63: if (hosts[i] == msg.sender) { 64, 64: isHost = true; 65, 65: break; 66, 66: } + 67:+ unchecked { + 68:+ ++i; + 69:+ } 67, 70: } 68, 71: 69, 72: if (!isHost) {
diff --git a/contracts/proposals/LibProposal.sol b/contracts/proposals/LibProposal.sol index 747d53d..75e2b29 100644 --- a/contracts/proposals/LibProposal.sol +++ b/contracts/proposals/LibProposal.sol @@ -11,10 +11,13 @@ library LibProposal { 11, 11: pure 12, 12: returns (bool) 13, 13: { - 14 :- for (uint256 i = 0; i < preciousTokens.length; ++i) { + 14:+ for (uint256 i = 0; i < preciousTokens.length; ) { 15, 15: if (token == preciousTokens[i]) { 16, 16: return true; 17, 17: } + 18:+ unchecked { + 19:+ ++i; + 20:+ } 18, 21: } 19, 22: return false; 20, 23: } @@ -29,10 +32,13 @@ library LibProposal { 29, 32: pure 30, 33: returns (bool) 31, 34: { - 32 :- for (uint256 i = 0; i < preciousTokens.length; ++i) { + 35:+ for (uint256 i = 0; i < preciousTokens.length;) { 33, 36: if (token == preciousTokens[i] && tokenId == preciousTokenIds[i]) { 34, 37: return true; 35, 38: } + 39:+ unchecked { + 40:+ ++i; + 41:+ } 36, 42: } 37, 43: return false; 38, 44: }
diff --git a/contracts/proposals/ListOnOpenseaProposal.sol b/contracts/proposals/ListOnOpenseaProposal.sol index 6ff8f5d..f2d7e98 100644 --- a/contracts/proposals/ListOnOpenseaProposal.sol +++ b/contracts/proposals/ListOnOpenseaProposal.sol @@ -288,13 +288,16 @@ abstract contract ListOnOpenseaProposal is ZoraHelpers { 288, 288: cons.identifierOrCriteria = 0; 289, 289: cons.startAmount = cons.endAmount = listPrice; 290, 290: cons.recipient = payable(address(this)); - 291 :- for (uint256 i = 0; i < fees.length; ++i) { + 291:+ for (uint256 i = 0; i < fees.length;) { 292, 292: cons = orderParams.consideration[1 + i]; 293, 293: cons.itemType = IOpenseaExchange.ItemType.NATIVE; 294, 294: cons.token = address(0); 295, 295: cons.identifierOrCriteria = 0; 296, 296: cons.startAmount = cons.endAmount = fees[i]; 297, 297: cons.recipient = feeRecipients[i]; + 298:+ unchecked { + 299:+ ++i; + 300:+ } 298, 301: } 299, 302: } 300, 303: orderHash = _getOrderHash(orderParams);
diff --git a/contracts/party/PartyGovernance.sol b/contracts/party/PartyGovernance.sol index 5dc6483..ba458db 100644 --- a/contracts/party/PartyGovernance.sol +++ b/contracts/party/PartyGovernance.sol @@ -303,8 +303,11 @@ abstract contract PartyGovernance is 303, 303: // Set the precious list. 304, 304: _setPreciousList(preciousTokens, preciousTokenIds); 305, 305: // Set the party hosts. - 306 :- for (uint256 i=0; i < opts.hosts.length; ++i) { + 306:+ for (uint256 i=0; i < opts.hosts.length;) { 307, 307: isHost[opts.hosts[i]] = true; + 308:+ unchecked { + 309:+ ++i; + 310:+ } 308, 311: } 309, 312: emit PartyInitialized(opts, preciousTokens, preciousTokenIds); 310, 313: } @@ -529,7 +532,9 @@ abstract contract PartyGovernance is 529, 532: onlyDelegateCall 530, 533: returns (uint256 proposalId) 531, 534: { - 532 :- proposalId = ++lastProposalId; + 535:+ unchecked { + 536:+ proposalId = ++lastProposalId; + 537:+ } 533, 538: // Store the time the proposal was created and the proposal hash. 534, 539: ( 535, 540: _proposalStateByProposalId[proposalId].values,
diff --git a/contracts/distribution/TokenDistributor.sol b/contracts/distribution/TokenDistributor.sol index 1ea71c1..21a1e4a 100644 --- a/contracts/distribution/TokenDistributor.sol +++ b/contracts/distribution/TokenDistributor.sol @@ -227,8 +227,11 @@ contract TokenDistributor is ITokenDistributor { 227, 227: returns (uint128[] memory amountsClaimed) 228, 228: { 229, 229: amountsClaimed = new uint128[](infos.length); - 230 :- for (uint256 i = 0; i < infos.length; ++i) { + 230:+ for (uint256 i = 0; i < infos.length;) { 231, 231: amountsClaimed[i] = claim(infos[i], partyTokenIds[i]); + 232:+ unchecked { + 233:+ ++i; + 234:+ } 232, 235: } 233, 236: } 234, 237: @@ -236,8 +239,11 @@ contract TokenDistributor is ITokenDistributor { 236, 239: function batchClaimFee(DistributionInfo[] calldata infos, address payable[] calldata recipients) 237, 240: external 238, 241: { - 239 :- for (uint256 i = 0; i < infos.length; ++i) { + 242:+ for (uint256 i = 0; i < infos.length;) { 240, 243: claimFee(infos[i], recipients[i]); + 244:+ unchecked { + 245:+ ++i; + 246:+ } 241, 247: } 242, 248: } 243, 249:
diff --git a/contracts/party/PartyGovernance.sol b/contracts/party/PartyGovernance.sol index ba458db..23c3780 100644 --- a/contracts/party/PartyGovernance.sol +++ b/contracts/party/PartyGovernance.sol @@ -440,7 +440,9 @@ abstract contract PartyGovernance is 440, 440: high = mid; 441, 441: } else { 442, 442: // Entry is older. This is our best guess for now. - 443 :- low = mid + 1; + 443:+ unchecked { + 444:+ low = mid + 1; + 445:+ } 444, 446: } 445, 447: } 446, 448:
Deployment. Gas Saved: 35 242
Minumal Method Call. Gas Saved: 109 101 ± 300 000
Average Method Call. Gas Saved: 137 433 ± 300 000
Maximum Method Call. Gas Saved: 359 680 ± 300 000
forge snapshot --diff. Gas Saved: 678 523
NOTE: this project tries to pack storage variables into a single slot, but sometimes (when bool is not used at the same time as the variables it is packed with) it will be more expensive than just using uint256.(Only cases where uint256 is cheaper are included)
// Booleans are more expensive than uint256 or any type that takes up a full // word because each write operation emits an extra SLOAD to first read the // slot's contents, replace the bits taken up by the boolean, and then write // back. This is the compiler's defense against contract upgrades and // pointer aliasing, and it cannot be disabled.
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from 'false' to 'true', after having been 'true' in the past
diff --git a/contracts/crowdfund/Crowdfund.sol b/contracts/crowdfund/Crowdfund.sol index 5c27346..6d43a06 100644 --- a/contracts/crowdfund/Crowdfund.sol +++ b/contracts/crowdfund/Crowdfund.sol @@ -103,7 +103,7 @@ abstract contract Crowdfund is ERC721Receiver, CrowdfundNFT { 103, 103: /// in bps, where 10,000 = 100%. 104, 104: uint16 public splitBps; 105, 105: // Whether the share for split recipient has been claimed through `burn()`. - 106 :- bool private _splitRecipientHasBurned; + 106:+ uint256 private _splitRecipientHasBurned; 107, 107: /// @notice Hash of party governance options passed into `initialize()`. 108, 108: /// Used to check whether the `GovernanceOpts` passed into 109, 109: /// `_createParty()` matches. @@ -455,10 +455,10 @@ abstract contract Crowdfund is ERC721Receiver, CrowdfundNFT { 455, 455: } 456, 456: // Split recipient can burn even if they don't have a token. 457, 457: if (contributor == splitRecipient) { - 458 :- if (_splitRecipientHasBurned) { + 458:+ if (1==_splitRecipientHasBurned) { 459, 459: revert SplitRecipientAlreadyBurnedError(); 460, 460: } - 461 :- _splitRecipientHasBurned = true; + 461:+ _splitRecipientHasBurned = 1; 462, 462: } 463, 463: // Revert if already burned or does not exist. 464, 464: if (splitRecipient != contributor || _doesTokenExistFor(contributor)) {
diff --git a/contracts/party/PartyGovernance.sol b/contracts/party/PartyGovernance.sol index 5dc6483..628e803 100644 --- a/contracts/party/PartyGovernance.sol +++ b/contracts/party/PartyGovernance.sol @@ -145,7 +145,7 @@ abstract contract PartyGovernance is 145, 145: // Hash of the proposal. 146, 146: bytes32 hash; 147, 147: // Whether a member has voted for (accepted) this proposal already. - 148 :- mapping (address => bool) hasVoted; + 148:+ mapping (address => uint256) hasVoted; 149, 149: } 150, 150: 151, 151: event Proposed( @@ -584,11 +584,11 @@ abstract contract PartyGovernance is 584, 584: } 585, 585: 586, 586: // Cannot vote twice. - 587 :- if (info.hasVoted[msg.sender]) { + 587:+ if (1==info.hasVoted[msg.sender]) { 588, 588: revert AlreadyVotedError(msg.sender); 589, 589: } 590, 590: // Mark the caller as having voted. - 591 :- info.hasVoted[msg.sender] = true; + 591:+ info.hasVoted[msg.sender] = 1; 592, 592: 593, 593: // Increase the total votes that have been cast on this proposal. 594, 594: uint96 votingPower = getVotingPowerAt(msg.sender, values.proposedTime, snapIndex);
diff --git a/contracts/party/PartyGovernance.sol b/contracts/party/PartyGovernance.sol index 628e803..8b28455 100644 --- a/contracts/party/PartyGovernance.sol +++ b/contracts/party/PartyGovernance.sol @@ -194,7 +194,7 @@ abstract contract PartyGovernance is 194, 194: IGlobals private immutable _GLOBALS; 195, 195: 196, 196: /// @notice Whether the DAO has emergency powers for this party. - 197 :- bool public emergencyExecuteDisabled; + 197:+ uint256 public emergencyExecuteDisabled; 198, 198: /// @notice Distribution fee bps. 199, 199: uint16 public feeBps; 200, 200: /// @notice Distribution fee recipient. @@ -256,7 +256,7 @@ abstract contract PartyGovernance is 256, 256: 257, 257: // Only if `emergencyExecuteDisabled` is not true. 258, 258: modifier onlyWhenEmergencyExecuteAllowed() { - 259 :- if (emergencyExecuteDisabled) { + 259:+ if (1==emergencyExecuteDisabled) { 260, 260: revert OnlyWhenEmergencyActionsAllowedError(); 261, 261: } 262, 262: _; @@ -798,7 +798,7 @@ abstract contract PartyGovernance is 798, 798: /// @notice Revoke the DAO's ability to call emergencyExecute(). 799, 799: /// @dev Either the DAO or the party host can call this. 800, 800: function disableEmergencyExecute() external onlyPartyDaoOrHost onlyDelegateCall { - 801 :- emergencyExecuteDisabled = true; + 801:+ emergencyExecuteDisabled = 1; 802, 802: } 803, 803: 804, 804: function _executeProposal( diff --git a/contracts/renderers/PartyGovernanceNFTRenderer.sol b/contracts/renderers/PartyGovernanceNFTRenderer.sol index e88fe38..7af05e4 100644 --- a/contracts/renderers/PartyGovernanceNFTRenderer.sol +++ b/contracts/renderers/PartyGovernanceNFTRenderer.sol @@ -18,7 +18,7 @@ contract PartyGovernanceNFTRenderer is IERC721Renderer { 18, 18: 19, 19: // The renderer is called via delegateCall, so we need to declare the storage layout. 20, 20: // Run `yarn layout` to generate the current layout. - 21 :- bool emergencyExecuteDisabled; + 21:+ uint256 emergencyExecuteDisabled; 22, 22: uint16 feeBps; 23, 23: address payable feeRecipient; 24, 24: bytes32 preciousListHash;
diff --git a/contracts/party/PartyGovernance.sol b/contracts/party/PartyGovernance.sol index 8b28455..57a4a86 100644 --- a/contracts/party/PartyGovernance.sol +++ b/contracts/party/PartyGovernance.sol @@ -204,7 +204,7 @@ abstract contract PartyGovernance is 204, 204: /// @notice The last proposal ID that was used. 0 means no proposals have been made. 205, 205: uint256 public lastProposalId; 206, 206: /// @notice Whether an address is a party host. - 207 :- mapping(address => bool) public isHost; + 207:+ mapping(address => uint256) public isHost; 208, 208: /// @notice The last person a voter delegated its voting power to. 209, 209: mapping(address => address) public delegationsByVoter; 210, 210: // Constant governance parameters, fixed from the inception of this party. @@ -215,7 +215,7 @@ abstract contract PartyGovernance is 215, 215: mapping(address => VotingPowerSnapshot[]) private _votingPowerSnapshotsByVoter; 216, 216: 217, 217: modifier onlyHost() { - 218 :- if (!isHost[msg.sender]) { + 218:+ if (0==isHost[msg.sender]) { 219, 219: revert OnlyPartyHostError(); 220, 220: } 221, 221: _; @@ -248,7 +248,7 @@ abstract contract PartyGovernance is 248, 248: // Only the party DAO multisig or a party host can call. 249, 249: modifier onlyPartyDaoOrHost() { 250, 250: address partyDao = _GLOBALS.getAddress(LibGlobals.GLOBAL_DAO_WALLET); - 251 :- if (msg.sender != partyDao && !isHost[msg.sender]) { + 251:+ if (msg.sender != partyDao && 0==isHost[msg.sender]) { 252, 252: revert OnlyPartyDaoOrHostError(msg.sender, partyDao); 253, 253: } 254, 254: _; @@ -304,7 +304,7 @@ abstract contract PartyGovernance is 304, 304: _setPreciousList(preciousTokens, preciousTokenIds); 305, 305: // Set the party hosts. 306, 306: for (uint256 i=0; i < opts.hosts.length; ++i) { - 307 :- isHost[opts.hosts[i]] = true; + 307:+ isHost[opts.hosts[i]] = 1; 308, 308: } 309, 309: emit PartyInitialized(opts, preciousTokens, preciousTokenIds); 310, 310: } @@ -459,12 +459,12 @@ abstract contract PartyGovernance is 459, 459: // 0 is a special case burn address. 460, 460: if (newPartyHost != address(0)) { 461, 461: // Cannot transfer host status to an existing host. - 462 :- if(isHost[newPartyHost]) { + 462:+ if(1==isHost[newPartyHost]) { 463, 463: revert InvalidNewHostError(); 464, 464: } - 465 :- isHost[newPartyHost] = true; + 465:+ isHost[newPartyHost] = 1; 466, 466: } - 467 :- isHost[msg.sender] = false; + 467:+ isHost[msg.sender] = 0; 468, 468: emit HostStatusTransferred(msg.sender, newPartyHost); 469, 469: } 470, 470: diff --git a/contracts/renderers/PartyGovernanceNFTRenderer.sol b/contracts/renderers/PartyGovernanceNFTRenderer.sol index 7af05e4..4930ba9 100644 --- a/contracts/renderers/PartyGovernanceNFTRenderer.sol +++ b/contracts/renderers/PartyGovernanceNFTRenderer.sol @@ -23,7 +23,7 @@ contract PartyGovernanceNFTRenderer is IERC721Renderer { 23, 23: address payable feeRecipient; 24, 24: bytes32 preciousListHash; 25, 25: uint256 lastProposalId; - 26 :- mapping(address => bool) isHost; + 26:+ mapping(address => uint256) isHost; 27, 27: mapping(address => address) delegationsByVoter; 28, 28: PartyGovernance.GovernanceValues _governanceValues; 29, 29: mapping(uint256 => PartyGovernance.ProposalState) _proposalStateByProposalId; diff --git a/sol-tests/party/PartyGovernanceUnit.t.sol b/sol-tests/party/PartyGovernanceUnit.t.sol index e762347..c5e9ad0 100644 --- a/sol-tests/party/PartyGovernanceUnit.t.sol +++ b/sol-tests/party/PartyGovernanceUnit.t.sol @@ -2022,10 +2022,10 @@ contract PartyGovernanceUnitTest is Test, TestUtils { 2022,2022: gov.abdicate(newHost); 2023,2023: 2024,2024: // Assert old host is no longer host -2025 :- assertEq(gov.isHost(host), false); + 2025:+ assertEq(gov.isHost(host), 0); 2026,2026: 2027,2027: // Assert new host is host -2028 :- assertEq(gov.isHost(newHost), true); + 2028:+ assertEq(gov.isHost(newHost), 1); 2029,2029: } 2030,2030: 2031,2031: // You cannot transfer host status to an existing host
diff --git a/contracts/distribution/TokenDistributor.sol b/contracts/distribution/TokenDistributor.sol index 1ea71c1..532f90e 100644 --- a/contracts/distribution/TokenDistributor.sol +++ b/contracts/distribution/TokenDistributor.sol @@ -27,7 +27,7 @@ contract TokenDistributor is ITokenDistributor { 27, 27: // Whether the distribution's feeRecipient has claimed its fee. 28, 28: bool wasFeeClaimed; 29, 29: // Whether a governance token has claimed its distribution share. - 30 :- mapping(uint256 => bool) hasPartyTokenClaimed; + 30:+ mapping(uint256 => uint256) hasPartyTokenClaimed; 31, 31: } 32, 32: 33, 33: // Arguments for `_createDistribution()`. @@ -152,11 +152,11 @@ contract TokenDistributor is ITokenDistributor { 152, 152: revert InvalidDistributionInfoError(info); 153, 153: } 154, 154: // The partyTokenId must not have claimed its distribution yet. - 155 :- if (state.hasPartyTokenClaimed[partyTokenId]) { + 155:+ if (1==state.hasPartyTokenClaimed[partyTokenId]) { 156, 156: revert DistributionAlreadyClaimedByPartyTokenError(info.distributionId, partyTokenId); 157, 157: } 158, 158: // Mark the partyTokenId as having claimed their distribution. - 159 :- state.hasPartyTokenClaimed[partyTokenId] = true; + 159:+ state.hasPartyTokenClaimed[partyTokenId] = 1; 160, 160: 161, 161: // Compute amount owed to partyTokenId. 162, 162: amountClaimed = getClaimAmount(info.party, info.memberSupply, partyTokenId); @@ -282,7 +282,7 @@ contract TokenDistributor is ITokenDistributor { 282, 282: external 283, 283: view returns (bool) 284, 284: { - 285 :- return _distributionStates[party][distributionId].hasPartyTokenClaimed[partyTokenId]; + 285:+ return 1==_distributionStates[party][distributionId].hasPartyTokenClaimed[partyTokenId]; 286, 286: } 287, 287: 288, 288: /// @inheritdoc ITokenDistributor
diff --git a/contracts/distribution/TokenDistributor.sol b/contracts/distribution/TokenDistributor.sol index 532f90e..e49f356 100644 --- a/contracts/distribution/TokenDistributor.sol +++ b/contracts/distribution/TokenDistributor.sol @@ -59,7 +59,7 @@ contract TokenDistributor is ITokenDistributor { 59, 59: IGlobals public immutable GLOBALS; 60, 60: 61, 61: /// @notice Whether the DAO is no longer allowed to call emergency functions. - 62 :- bool public emergencyActionsDisabled; + 62:+ uint256 public emergencyActionsDisabled; 63, 63: /// @notice Last distribution ID for a party. 64, 64: mapping(ITokenDistributorParty => uint256) public lastDistributionIdPerParty; 65, 65: /// Last known balance of a token, identified by an ID derived from the token. @@ -83,7 +83,7 @@ contract TokenDistributor is ITokenDistributor { 83, 83: 84, 84: // emergencyActionsDisabled == false 85, 85: modifier onlyIfEmergencyActionsAllowed() { - 86 :- if (emergencyActionsDisabled) { + 86:+ if (1==emergencyActionsDisabled) { 87, 87: revert EmergencyActionsNotAllowedError(); 88, 88: } 89, 89: _; @@ -325,7 +325,7 @@ contract TokenDistributor is ITokenDistributor { 325, 325: 326, 326: /// @notice DAO-only function to disable emergency functions forever. 327, 327: function disableEmergencyActions() onlyPartyDao external { - 328 :- emergencyActionsDisabled = true; + 328:+ emergencyActionsDisabled = 1; 329, 329: } 330, 330: 331, 331: function _createDistribution(CreateDistributionArgs memory args)
Deployment. Gas Saved: 21 616
Minumal Method Call. Gas Saved: 23 247 ± 300 000
Average Method Call. Gas Saved: 45 963 ± 300 000
Maximum Method Call. Gas Saved: 357 682 ± 300 000
forge snapshot --diff. Gas Saved: 116 899
SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.
diff --git a/contracts/crowdfund/Crowdfund.sol b/contracts/crowdfund/Crowdfund.sol index 5c27346..5b38227 100644 --- a/contracts/crowdfund/Crowdfund.sol +++ b/contracts/crowdfund/Crowdfund.sol @@ -268,13 +268,14 @@ abstract contract Crowdfund is ERC721Receiver, CrowdfundNFT { 268, 268: internal 269, 269: returns (Party party_) 270, 270: { - 271 :- if (party != Party(payable(0))) { - 272 :- revert PartyAlreadyExistsError(party); + 271:+ if ((party_ = party) != Party(payable(0))) { + 272:+ revert PartyAlreadyExistsError(party_); 273, 273: } 274, 274: { 275, 275: bytes16 governanceOptsHash_ = _hashFixedGovernanceOpts(governanceOpts); - 276 :- if (governanceOptsHash_ != governanceOptsHash) { - 277 :- revert InvalidGovernanceOptionsError(governanceOptsHash_, governanceOptsHash); + 276:+ bytes32 _governanceOptsHash; + 277:+ if (governanceOptsHash_ != (_governanceOptsHash = governanceOptsHash)) { + 278:+ revert InvalidGovernanceOptionsError(governanceOptsHash_, _governanceOptsHash); 278, 279: } 279, 280: } 280, 281: party = party_ = partyFactory @@ -389,11 +390,12 @@ abstract contract Crowdfund is ERC721Receiver, CrowdfundNFT { 389, 390: revert InvalidDelegateError(); 390, 391: } 391, 392: // Must not be blocked by gatekeeper. - 392 :- if (gateKeeper != IGateKeeper(address(0))) { - 393 :- if (!gateKeeper.isAllowed(contributor, gateKeeperId, gateData)) { + 393:+ IGateKeeper _gateKeeper; + 394:+ if ((_gateKeeper = gateKeeper) != IGateKeeper(address(0))) { + 395:+ if (!_gateKeeper.isAllowed(contributor, gateKeeperId, gateData)) { 394, 396: revert NotAllowedByGateKeeperError( 395, 397: contributor, - 396 :- gateKeeper, + 398:+ _gateKeeper, 397, 399: gateKeeperId, 398, 400: gateData 399, 401: );
diff --git a/contracts/crowdfund/AuctionCrowdfund.sol b/contracts/crowdfund/AuctionCrowdfund.sol index e997e8a..8c9bc73 100644 --- a/contracts/crowdfund/AuctionCrowdfund.sol +++ b/contracts/crowdfund/AuctionCrowdfund.sol @@ -159,15 +159,16 @@ contract AuctionCrowdfund is Implementation, Crowdfund { 159, 159: _bidStatus = AuctionCrowdfundStatus.Busy; 160, 160: // Make sure the auction is not finalized. 161, 161: uint256 auctionId_ = auctionId; - 162 :- if (market.isFinalized(auctionId_)) { + 162:+ IMarketWrapper _market; + 163:+ if ((_market=market).isFinalized(auctionId_)) { 163, 164: revert AuctionFinalizedError(auctionId_); 164, 165: } 165, 166: // Only bid if we are not already the highest bidder. - 166 :- if (market.getCurrentHighestBidder(auctionId_) == address(this)) { + 167:+ if (_market.getCurrentHighestBidder(auctionId_) == address(this)) { 167, 168: revert AlreadyHighestBidderError(); 168, 169: } 169, 170: // Get the minimum necessary bid to be the highest bidder. - 170 :- uint96 bidAmount = market.getMinimumBid(auctionId_).safeCastUint256ToUint96(); + 171:+ uint96 bidAmount = _market.getMinimumBid(auctionId_).safeCastUint256ToUint96(); 171, 172: // Make sure the bid is less than the maximum bid. 172, 173: if (bidAmount > maximumBid) { 173, 174: revert ExceedsMaximumBidError(bidAmount, maximumBid); @@ -175,7 +176,7 @@ contract AuctionCrowdfund is Implementation, Crowdfund { 175, 176: lastBid = bidAmount; 176, 177: // No need to check that we have `bidAmount` since this will attempt to 177, 178: // transfer `bidAmount` ETH to the auction platform. - 178 :- (bool s, bytes memory r) = address(market).delegatecall(abi.encodeCall( + 179:+ (bool s, bytes memory r) = address(_market).delegatecall(abi.encodeCall( 179, 180: IMarketWrapper.bid, 180, 181: (auctionId_, bidAmount) 181, 182: ));
diff --git a/contracts/crowdfund/BuyCrowdfundBase.sol b/contracts/crowdfund/BuyCrowdfundBase.sol index e886e57..fa0d445 100644 --- a/contracts/crowdfund/BuyCrowdfundBase.sol +++ b/contracts/crowdfund/BuyCrowdfundBase.sol @@ -115,7 +115,7 @@ abstract contract BuyCrowdfundBase is Implementation, Crowdfund { 115, 115: { 116, 116: uint96 maximumPrice_ = maximumPrice; 117, 117: if (maximumPrice_ != 0 && callValue > maximumPrice_) { - 118 :- revert MaximumPriceError(callValue, maximumPrice); + 118:+ revert MaximumPriceError(callValue, maximumPrice_); 119, 119: } 120, 120: // If the purchase would be free, set the settled price to 121, 121: // `totalContributions` so everybody who contributed wins.
Deployment. Gas Saved: 18 424
Minumal Method Call. Gas Saved: 20 249 ± 300 000
Average Method Call. Gas Saved: -3 095 ± 300 000
Maximum Method Call. Gas Saved: -114 589 ± 300 000
forge snapshot --diff. Gas Saved: 126 535
Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they're hitby avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas
diff --git a/contracts/crowdfund/CrowdfundNFT.sol b/contracts/crowdfund/CrowdfundNFT.sol index 8c6208f..02bc2c6 100644 --- a/contracts/crowdfund/CrowdfundNFT.sol +++ b/contracts/crowdfund/CrowdfundNFT.sol @@ -11,6 +11,7 @@ import "../globals/LibGlobals.sol"; 11, 11: contract CrowdfundNFT is IERC721, EIP165, ReadOnlyDelegateCall { 12, 12: error AlreadyBurnedError(address owner, uint256 tokenId); 13, 13: error InvalidTokenError(uint256 tokenId); + 14:+ error AlwaysRevert(); 14, 15: 15, 16: // The `Globals` contract storing global configuration values. This contract 16, 17: // is immutable and it’s address will never change. @@ -26,7 +27,7 @@ contract CrowdfundNFT is IERC721, EIP165, ReadOnlyDelegateCall { 26, 27: mapping (uint256 => address) private _owners; 27, 28: 28, 29: modifier alwaysRevert() { - 29 :- revert('ALWAYS FAILING'); + 30:+ revert AlwaysRevert(); 30, 31: _; // Compiler requires this. 31, 32: } 32, 33:
<array>.length should not be looked up in every loop of a for-loop (3 instances)Deployment. Gas Saved: 8 200
Minumal Method Call. Gas Saved: 69 470 ± 300 000
Average Method Call. Gas Saved: 62 475 ± 300 000
Maximum Method Call. Gas Saved: 48 162 ± 300 000
forge snapshot --diff. Gas Saved: 17 105
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
diff --git a/contracts/proposals/ArbitraryCallsProposal.sol b/contracts/proposals/ArbitraryCallsProposal.sol index ec164bc..7f933c5 100644 --- a/contracts/proposals/ArbitraryCallsProposal.sol +++ b/contracts/proposals/ArbitraryCallsProposal.sol @@ -49,7 +49,8 @@ contract ArbitraryCallsProposal { 49, 49: // so we can check that we still have them later. 50, 50: bool[] memory hadPreciouses = new bool[](params.preciousTokenIds.length); 51, 51: if (!isUnanimous) { - 52 :- for (uint256 i = 0; i < hadPreciouses.length; ++i) { + 52:+ uint256 len = hadPreciouses.length; + 53:+ for (uint256 i = 0; i < len; ++i) { 53, 54: hadPreciouses[i] = _getHasPrecious( 54, 55: params.preciousTokens[i], 55, 56: params.preciousTokenIds[i]
diff --git a/contracts/party/PartyGovernance.sol b/contracts/party/PartyGovernance.sol index 5dc6483..cbbf3b4 100644 --- a/contracts/party/PartyGovernance.sol +++ b/contracts/party/PartyGovernance.sol @@ -303,7 +303,8 @@ abstract contract PartyGovernance is 303, 303: // Set the precious list. 304, 304: _setPreciousList(preciousTokens, preciousTokenIds); 305, 305: // Set the party hosts. - 306 :- for (uint256 i=0; i < opts.hosts.length; ++i) { + 306:+ uint256 len = opts.hosts.length; + 307:+ for (uint256 i=0; i < len; ++i) { 307, 308: isHost[opts.hosts[i]] = true; 308, 309: } 309, 310: emit PartyInitialized(opts, preciousTokens, preciousTokenIds);
diff --git a/contracts/crowdfund/Crowdfund.sol b/contracts/crowdfund/Crowdfund.sol index 5c27346..e167e39 100644 --- a/contracts/crowdfund/Crowdfund.sol +++ b/contracts/crowdfund/Crowdfund.sol @@ -177,7 +177,8 @@ abstract contract Crowdfund is ERC721Receiver, CrowdfundNFT { 177, 177: { 178, 178: Party party_ = party; 179, 179: CrowdfundLifecycle lc = getCrowdfundLifecycle(); - 180 :- for (uint256 i = 0; i < contributors.length; ++i) { + 180:+ uint256 len = contributors.length; + 181:+ for (uint256 i = 0; i < len; ++i) { 181, 182: _burn(contributors[i], lc, party_); 182, 183: } 183, 184: }
Deployment. Gas Saved: 1 600
Minumal Method Call. Gas Saved: 203 819 ± 300 000
Average Method Call. Gas Saved: 222 938 ± 300 000
Maximum Method Call. Gas Saved: 313 121 ± 300 000
forge snapshot --diff. Gas Saved: 87 318
<x> / 2 is the same as <x> >> 1. The DIV opcode costs 5 gas, whereas SHR only costs 3 gas
diff --git a/contracts/party/PartyGovernance.sol b/contracts/party/PartyGovernance.sol index 5dc6483..cb21f7c 100644 --- a/contracts/party/PartyGovernance.sol +++ b/contracts/party/PartyGovernance.sol @@ -431,7 +431,7 @@ abstract contract PartyGovernance is 431, 431: uint256 high = snaps.length; 432, 432: uint256 low = 0; 433, 433: while (low < high) { - 434 :- uint256 mid = (low + high) / 2; + 434:+ uint256 mid = (low + high) >> 1; 435, 435: if (snaps[mid].timestamp > timestamp) { 436, 436: // Entry is too recent. 437, 437: high = mid;
Deployment. Gas Saved: 600
Minumal Method Call. Gas Saved: -21986 ± 300 000
Average Method Call. Gas Saved: -33567 ± 300 000
Maximum Method Call. Gas Saved: 44774 ± 300 000
forge snapshot --diff. Gas Saved: 616
diff --git a/contracts/crowdfund/Crowdfund.sol b/contracts/crowdfund/Crowdfund.sol index 5c27346..de29ea6 100644 --- a/contracts/crowdfund/Crowdfund.sol +++ b/contracts/crowdfund/Crowdfund.sol @@ -236,12 +236,12 @@ abstract contract Crowdfund is ERC721Receiver, CrowdfundNFT { 236, 236: uint256 votingPower 237, 237: ) 238, 238: { - 239 :- CrowdfundLifecycle lc = getCrowdfundLifecycle(); 240, 239: Contribution[] storage contributions = _contributionsByContributor[contributor]; 241, 240: uint256 numContributions = contributions.length; 242, 241: for (uint256 i = 0; i < numContributions; ++i) { 243, 242: ethContributed += contributions[i].amount; 244, 243: } + 244:+ CrowdfundLifecycle lc = getCrowdfundLifecycle(); 245, 245: if (lc == CrowdfundLifecycle.Won || lc == CrowdfundLifecycle.Lost) { 246, 246: (ethUsed, ethOwed, votingPower) = _getFinalContribution(contributor); 247, 247: }
Deployment. Gas Saved: 400
Minumal Method Call. Gas Saved: -22938 ± 300 000
Average Method Call. Gas Saved: -49907 ± 300 000
Maximum Method Call. Gas Saved: -246113 ± 300 000
forge snapshot --diff. Gas Saved: 5
diff --git a/contracts/crowdfund/CollectionBuyCrowdfund.sol b/contracts/crowdfund/CollectionBuyCrowdfund.sol index a0517eb..73b7e66 100644 --- a/contracts/crowdfund/CollectionBuyCrowdfund.sol +++ b/contracts/crowdfund/CollectionBuyCrowdfund.sol @@ -59,7 +59,7 @@ contract CollectionBuyCrowdfund is BuyCrowdfundBase { 59, 59: 60, 60: modifier onlyHost(address[] memory hosts) { 61, 61: bool isHost; - 62 :- for (uint256 i; i < hosts.length; i++) { + 62:+ for (uint256 i; i < hosts.length; ++i) { 63, 63: if (hosts[i] == msg.sender) { 64, 64: isHost = true; 65, 65: break;
#0 - 0xble
2022-09-26T01:38:21Z
Unhelpful bot report