refactors: QAs + gas optimizations (#142)

* refactors + optimizations

* style: tests

* refactor

* rebase

Co-authored-by: Lawrence Forman <me@merklejerk.com>

contracts/crowdfund/AuctionCrowdfund.sol

@@ -84,6 +84,7 @@ contract AuctionCrowdfund is Crowdfund {
     error AuctionFinalizedError(uint256 auctionId);
     error AlreadyHighestBidderError();
     error ExceedsMaximumBidError(uint256 bidAmount, uint256 maximumBid);
+    error MinimumBidExceedsMaximumBidError(uint256 bidAmount, uint256 maximumBid);
     error NoContributionsError();
     error AuctionNotExpiredError();
 
@@ -151,6 +152,12 @@ contract AuctionCrowdfund is Crowdfund {
         {
             revert InvalidAuctionIdError();
         }
+
+        // Check that the minimum bid is less than the maximum bid.
+        uint256 minimumBid = market.getMinimumBid(opts.auctionId);
+        if (minimumBid > opts.maximumBid) {
+            revert MinimumBidExceedsMaximumBidError(minimumBid, opts.maximumBid);
+        }
     }
 
     /// @notice Accept naked ETH, e.g., if an auction needs to return ETH to us.
@@ -196,33 +203,35 @@ contract AuctionCrowdfund is Crowdfund {
         _bidStatus = AuctionCrowdfundStatus.Busy;
 
         // Make sure the auction is not finalized.
+        IMarketWrapper market_ = market;
         uint256 auctionId_ = auctionId;
-        if (market.isFinalized(auctionId_)) {
+        if (market_.isFinalized(auctionId_)) {
             revert AuctionFinalizedError(auctionId_);
         }
 
         // Only bid if we are not already the highest bidder.
-        if (market.getCurrentHighestBidder(auctionId_) == address(this)) {
+        if (market_.getCurrentHighestBidder(auctionId_) == address(this)) {
             revert AlreadyHighestBidderError();
         }
 
         // Get the minimum necessary bid to be the highest bidder.
-        uint96 bidAmount = market.getMinimumBid(auctionId_).safeCastUint256ToUint96();
+        uint96 bidAmount = market_.getMinimumBid(auctionId_).safeCastUint256ToUint96();
         // Prevent unaccounted ETH from being used to inflate the bid and
         // create "ghost shares" in voting power.
         uint96 totalContributions_ = totalContributions;
         if (bidAmount > totalContributions_) {
             revert ExceedsTotalContributionsError(bidAmount, totalContributions_);
         }
         // Make sure the bid is less than the maximum bid.
-        if (bidAmount > maximumBid) {
-            revert ExceedsMaximumBidError(bidAmount, maximumBid);
+        uint96 maximumBid_ = maximumBid;
+        if (bidAmount > maximumBid_) {
+            revert ExceedsMaximumBidError(bidAmount, maximumBid_);
         }
         lastBid = bidAmount;
 
         // No need to check that we have `bidAmount` since this will attempt to
         // transfer `bidAmount` ETH to the auction platform.
-        (bool s, bytes memory r) = address(market).delegatecall(abi.encodeCall(
+        (bool s, bytes memory r) = address(market_).delegatecall(abi.encodeCall(
             IMarketWrapper.bid,
             (auctionId_, bidAmount)
         ));
@@ -253,23 +262,27 @@ contract AuctionCrowdfund is Crowdfund {
         // Mark as busy to prevent burn(), bid(), and contribute()
         // getting called because this will result in a `CrowdfundLifecycle.Busy`.
         _bidStatus = AuctionCrowdfundStatus.Busy;
-
-        uint256 auctionId_ = auctionId;
-        // Finalize the auction if it isn't finalized.
-        if (!market.isFinalized(auctionId_)) {
-            // Note that even if this crowdfund has expired but the auction is still
-            // ongoing, this call can fail and block finalization until the auction ends.
-            (bool s, bytes memory r) = address(market).call(abi.encodeCall(
-                IMarketWrapper.finalize,
-                auctionId_
-            ));
-            if (!s) {
-                r.rawRevert();
+        {
+            uint256 auctionId_ = auctionId;
+            IMarketWrapper market_ = market;
+            // Finalize the auction if it isn't finalized.
+            if (!market_.isFinalized(auctionId_)) {
+                // Note that even if this crowdfund has expired but the auction is still
+                // ongoing, this call can fail and block finalization until the auction ends.
+                (bool s, bytes memory r) = address(market_).call(abi.encodeCall(
+                    IMarketWrapper.finalize,
+                    auctionId_
+                ));
+                if (!s) {
+                    r.rawRevert();
+                }
             }
         }
+        IERC721 nftContract_ = nftContract;
+        uint256 nftTokenId_ = nftTokenId;
         if (
             // Are we now in possession of the NFT?
-            nftContract.safeOwnerOf(nftTokenId) == address(this) &&
+            nftContract_.safeOwnerOf(nftTokenId_) == address(this) &&
             // And it wasn't acquired for free or "gifted" to us?
             address(this).balance < totalContributions
         ) {

contracts/crowdfund/BuyCrowdfund.sol

@@ -5,7 +5,6 @@ pragma solidity 0.8.17;
 import "../tokens/IERC721.sol";
 import "../party/Party.sol";
 import "../utils/LibSafeERC721.sol";
-import "../utils/LibRawResult.sol";
 import "../globals/IGlobals.sol";
 import "../gatekeepers/IGateKeeper.sol";
 

contracts/crowdfund/BuyCrowdfundBase.sol

@@ -29,9 +29,8 @@ abstract contract BuyCrowdfundBase is Crowdfund {
         // How long this crowdfund has to bid on the NFT, in seconds.
         uint40 duration;
         // Maximum amount this crowdfund will pay for the NFT.
-        // If zero, no maximum.
         uint96 maximumPrice;
-        // An address that receieves an extra share of the final voting power
+        // An address that receives an extra share of the final voting power
         // when the party transitions into governance.
         address payable splitRecipient;
         // What percentage (in bps) of the final total voting power `splitRecipient`

contracts/crowdfund/Crowdfund.sol

@@ -193,7 +193,7 @@ abstract contract Crowdfund is Implementation, ERC721Receiver, CrowdfundNFT {
     function batchBurn(address payable[] calldata contributors) external {
         Party party_ = party;
         CrowdfundLifecycle lc = getCrowdfundLifecycle();
-        for (uint256 i = 0; i < contributors.length; ++i) {
+        for (uint256 i; i < contributors.length; ++i) {
             _burn(contributors[i], lc, party_);
         }
     }
@@ -277,9 +277,9 @@ abstract contract Crowdfund is Implementation, ERC721Receiver, CrowdfundNFT {
         )
     {
         CrowdfundLifecycle lc = getCrowdfundLifecycle();
-        Contribution[] storage contributions = _contributionsByContributor[contributor];
+        Contribution[] memory contributions = _contributionsByContributor[contributor];
         uint256 numContributions = contributions.length;
-        for (uint256 i = 0; i < numContributions; ++i) {
+        for (uint256 i; i < numContributions; ++i) {
             ethContributed += contributions[i].amount;
         }
         if (lc == CrowdfundLifecycle.Won || lc == CrowdfundLifecycle.Lost) {
@@ -288,7 +288,7 @@ abstract contract Crowdfund is Implementation, ERC721Receiver, CrowdfundNFT {
     }
 
     /// @notice Get the current lifecycle of the crowdfund.
-    function getCrowdfundLifecycle() public virtual view returns (CrowdfundLifecycle);
+    function getCrowdfundLifecycle() public virtual view returns (CrowdfundLifecycle lifecycle);
 
     // Get the final sale price of the bought assets. This will also be the total
     // voting power of the governance party.
@@ -349,7 +349,7 @@ abstract contract Crowdfund is Implementation, ERC721Receiver, CrowdfundNFT {
     }
 
     // Can be called after a party has won.
-    // Deploys and initializes a a `Party` instance via the `PartyFactory`
+    // Deploys and initializes a `Party` instance via the `PartyFactory`
     // and transfers the bought NFT to it.
     // After calling this, anyone can burn CF tokens on a contributor's behalf
     // with the `burn()` function.
@@ -393,7 +393,7 @@ abstract contract Crowdfund is Implementation, ERC721Receiver, CrowdfundNFT {
                 preciousTokenIds
             );
         // Transfer the acquired NFTs to the new party.
-        for (uint256 i = 0; i < preciousTokens.length; ++i) {
+        for (uint256 i; i < preciousTokens.length; ++i) {
             preciousTokens[i].transferFrom(address(this), address(party_), preciousTokenIds[i]);
         }
     }
@@ -435,7 +435,7 @@ abstract contract Crowdfund is Implementation, ERC721Receiver, CrowdfundNFT {
         assembly {
             // Replace the address[] hosts field with its hash temporarily.
             let oldHostsFieldValue := mload(opts)
-            mstore(opts, keccak256(add(mload(opts), 0x20), mul(mload(mload(opts)), 32)))
+            mstore(opts, keccak256(add(oldHostsFieldValue, 0x20), mul(mload(oldHostsFieldValue), 32)))
             // Hash the entire struct.
             h := keccak256(opts, 0xC0)
             // Restore old hosts field value.
@@ -450,9 +450,9 @@ abstract contract Crowdfund is Implementation, ERC721Receiver, CrowdfundNFT {
     {
         uint256 totalEthUsed = _getFinalPrice();
         {
-            Contribution[] storage contributions = _contributionsByContributor[contributor];
+            Contribution[] memory contributions = _contributionsByContributor[contributor];
             uint256 numContributions = contributions.length;
-            for (uint256 i = 0; i < numContributions; ++i) {
+            for (uint256 i; i < numContributions; ++i) {
                 Contribution memory c = contributions[i];
                 if (c.previousTotalContributions >= totalEthUsed) {
                     // This entire contribution was not used.
@@ -496,11 +496,12 @@ abstract contract Crowdfund is Implementation, ERC721Receiver, CrowdfundNFT {
             revert InvalidDelegateError();
         }
         // Must not be blocked by gatekeeper.
-        if (gateKeeper != IGateKeeper(address(0))) {
-            if (!gateKeeper.isAllowed(contributor, gateKeeperId, gateData)) {
+        IGateKeeper _gateKeeper = gateKeeper;
+        if (_gateKeeper != IGateKeeper(address(0))) {
+            if (!_gateKeeper.isAllowed(contributor, gateKeeperId, gateData)) {
                 revert NotAllowedByGateKeeperError(
                     contributor,
-                    gateKeeper,
+                    _gateKeeper,
                     gateKeeperId,
                     gateData
                 );
@@ -514,16 +515,15 @@ abstract contract Crowdfund is Implementation, ERC721Receiver, CrowdfundNFT {
 
         // OK to contribute with zero just to update delegate.
         if (amount != 0) {
-            // Increase total contributions.
-            totalContributions += amount;
-
             // Only allow contributions while the crowdfund is active.
             {
                 CrowdfundLifecycle lc = getCrowdfundLifecycle();
                 if (lc != CrowdfundLifecycle.Active) {
                     revert WrongLifecycleError(lc);
                 }
             }
+            // Increase total contributions.
+            totalContributions += amount;
             // Create contributions entry for this contributor.
             Contribution[] storage contributions = _contributionsByContributor[contributor];
             uint256 numContributions = contributions.length;
@@ -562,15 +562,18 @@ abstract contract Crowdfund is Implementation, ERC721Receiver, CrowdfundNFT {
             revert WrongLifecycleError(lc);
         }
         // Split recipient can burn even if they don't have a token.
-        if (contributor == splitRecipient) {
-            if (_splitRecipientHasBurned) {
-                revert SplitRecipientAlreadyBurnedError();
+        {
+            address splitRecipient_ = splitRecipient;
+            if (contributor == splitRecipient_) {
+                if (_splitRecipientHasBurned) {
+                    revert SplitRecipientAlreadyBurnedError();
+                }
+                _splitRecipientHasBurned = true;
+            }
+            // Revert if already burned or does not exist.
+            if (splitRecipient_ != contributor || _doesTokenExistFor(contributor)) {
+                CrowdfundNFT._burn(contributor);
             }
-            _splitRecipientHasBurned = true;
-        }
-        // Revert if already burned or does not exist.
-        if (splitRecipient != contributor || _doesTokenExistFor(contributor)) {
-            CrowdfundNFT._burn(contributor);
         }
         // Compute the contributions used and owed to the contributor, along
         // with the voting power they'll have in the governance stage.

contracts/crowdfund/CrowdfundFactory.sol

@@ -28,7 +28,7 @@ contract CrowdfundFactory {
         _GLOBALS = globals;
     }
 
-    /// @notice Create a new crowdfund to purchases a specific NFT (i.e., with a
+    /// @notice Create a new crowdfund to purchase a specific NFT (i.e., with a
     ///         known token ID) listing for a known price.
     /// @param opts Options used to initialize the crowdfund. These are fixed
     ///             and cannot be changed later.
@@ -80,7 +80,7 @@ contract CrowdfundFactory {
         emit AuctionCrowdfundCreated(inst, opts);
     }
 
-    /// @notice Create a new crowdfund to purchases any NFT from a collection
+    /// @notice Create a new crowdfund to purchase any NFT from a collection
     ///         (i.e. any token ID) from a collection for a known price.
     /// @param opts Options used to initialize the crowdfund. These are fixed
     ///             and cannot be changed later.

contracts/crowdfund/CrowdfundNFT.sol

@@ -11,6 +11,7 @@ import "../renderers/RendererStorage.sol";
 
 /// @notice NFT functionality for crowdfund types. This NFT is soulbound and read-only.
 contract CrowdfundNFT is IERC721, EIP165, ReadOnlyDelegateCall {
+    error AlreadyMintedError(address owner, uint256 tokenId);
     error AlreadyBurnedError(address owner, uint256 tokenId);
     error InvalidTokenError(uint256 tokenId);
     error InvalidAddressError();
@@ -59,34 +60,39 @@ contract CrowdfundNFT is IERC721, EIP165, ReadOnlyDelegateCall {
     ///         Attempting to call this function will always fail.
     function transferFrom(address, address, uint256)
         external
+        pure
         alwaysRevert
     {}
 
     /// @notice DO NOT CALL. This is a soulbound NFT and cannot be transferred.
     ///         Attempting to call this function will always fail.
     function safeTransferFrom(address, address, uint256)
         external
+        pure
         alwaysRevert
     {}
 
     /// @notice DO NOT CALL. This is a soulbound NFT and cannot be transferred.
     ///         Attempting to call this function will always fail.
     function safeTransferFrom(address, address, uint256, bytes calldata)
         external
+        pure
         alwaysRevert
     {}
 
     /// @notice DO NOT CALL. This is a soulbound NFT and cannot be transferred.
     ///         Attempting to call this function will always fail.
     function approve(address, uint256)
         external
+        pure
         alwaysRevert
     {}
 
     /// @notice DO NOT CALL. This is a soulbound NFT and cannot be transferred.
     ///         Attempting to call this function will always fail.
     function setApprovalForAll(address, bool)
         external
+        pure
         alwaysRevert
     {}
 
@@ -155,6 +161,8 @@ contract CrowdfundNFT is IERC721, EIP165, ReadOnlyDelegateCall {
         if (_owners[tokenId] != owner) {
             _owners[tokenId] = owner;
             emit Transfer(address(0), owner, tokenId);
+        } else {
+            revert AlreadyMintedError(owner, tokenId);
         }
     }
 

contracts/distribution/ITokenDistributor.sol

@@ -59,7 +59,7 @@ interface ITokenDistributor {
     /// @param party The party whose members can claim the distribution.
     /// @param feeRecipient Who can claim `fee`.
     /// @param feeBps Percentage (in bps) of the distribution `feeRecipient` receives.
-    /// @param info Information on the created distribution.
+    /// @return info Information on the created distribution.
     function createNativeDistribution(
         ITokenDistributorParty party,
         address payable feeRecipient,
@@ -78,7 +78,7 @@ interface ITokenDistributor {
     /// @param party The party whose members can claim the distribution.
     /// @param feeRecipient Who can claim `fee`.
     /// @param feeBps Percentage (in bps) of the distribution `feeRecipient` receives.
-    /// @param info Information on the created distribution.
+    /// @return info Information on the created distribution.
     function createErc20Distribution(
         IERC20 token,
         ITokenDistributorParty party,
@@ -93,7 +93,7 @@ interface ITokenDistributor {
     ///         must own this token.
     /// @param info Information on the distribution being claimed.
     /// @param partyTokenId The ID of the party token to claim for.
-    /// @param amountClaimed The amount of the distribution claimed.
+    /// @return amountClaimed The amount of the distribution claimed.
     function claim(DistributionInfo calldata info, uint256 partyTokenId)
         external
         returns (uint128 amountClaimed);
@@ -108,7 +108,7 @@ interface ITokenDistributor {
     /// @notice Batch version of `claim()`.
     /// @param infos Information on the distributions being claimed.
     /// @param partyTokenIds The ID of the party tokens to claim for.
-    /// @param amountsClaimed The amount of the distributions claimed.
+    /// @return amountsClaimed The amount of the distributions claimed.
     function batchClaim(DistributionInfo[] calldata infos, uint256[] calldata partyTokenIds)
         external
         returns (uint128[] memory amountsClaimed);

contracts/distribution/TokenDistributor.sol

@@ -41,8 +41,9 @@ contract TokenDistributor is ITokenDistributor {
         uint16 feeBps;
     }
 
+    event EmergencyExecute(address target, bytes data);
+
     error OnlyPartyDaoError(address notDao, address partyDao);
-    error OnlyPartyDaoAuthorityError(address notDaoAuthority);
     error InvalidDistributionInfoError(DistributionInfo info);
     error DistributionAlreadyClaimedByPartyTokenError(uint256 distributionId, uint256 partyTokenId);
     error DistributionFeeAlreadyClaimedError(uint256 distributionId);
@@ -315,6 +316,7 @@ contract TokenDistributor is ITokenDistributor {
         if (!success) {
             res.rawRevert();
         }
+        emit EmergencyExecute(targetAddress, targetCallData);
     }
 
     function _createDistribution(CreateDistributionArgs memory args)