fix: Handling Reentrancy (#108)

* Move GeoWebParcelFacet to new version pattern * Update PCOLicenseDiamond with new versioning and interfaces * Remove .direnv * Clean up some overrides and imports * Update solcover excludes * fix: Move ERC721 mint to end * Add comment about reclaim reentrancy * Move license transfer to end of triggerTransfer * Move delete payer flow to end of triggerTransfer()
Geo-Web-Project · Nov 18, 2022 · f0650af · f0650af
1 parent 17fa040
commit f0650af
Show file tree

Hide file tree

Showing 7 changed files with 73 additions and 53 deletions.
diff --git a/contracts/pco-license/facets/CFAPenaltyBidFacet.sol b/contracts/pco-license/facets/CFAPenaltyBidFacet.sol
@@ -253,6 +253,7 @@ contract CFAPenaltyBidFacet is ICFAPenaltyBid, CFABasePCOFacetModifiers {
             _currentBid.forSalePrice
         );
 
+        // Reentrancy on ERC721 transfer
         LibCFAPenaltyBid._triggerTransfer();
     }
 
@@ -301,6 +302,7 @@ contract CFAPenaltyBidFacet is ICFAPenaltyBid, CFABasePCOFacetModifiers {
             _currentBid.forSalePrice
         );
 
+        // Reentrancy on ERC721 transfer
         LibCFAPenaltyBid._triggerTransfer();
     }
 }
diff --git a/contracts/pco-license/facets/CFAReclaimerFacet.sol b/contracts/pco-license/facets/CFAReclaimerFacet.sol
@@ -177,7 +177,7 @@ contract CFAReclaimerFacet is ICFAReclaimer, CFABasePCOFacetModifiers {
         // Update beneficiary flow
         LibCFABasePCO._createBeneficiaryFlow(newContributionRate);
 
-        // Transfer license
+        // Transfer license (reentrancy on ERC721 transfer)
         ds.license.safeTransferFrom(bidder, msg.sender, ds.licenseId);
     }
 }
diff --git a/contracts/pco-license/libraries/LibCFAPenaltyBid.sol b/contracts/pco-license/libraries/LibCFAPenaltyBid.sol
@@ -97,20 +97,6 @@ library LibCFAPenaltyBid {
         // Update pending bid
         _clearPendingBid();
 
-        // Delete payer flow
-        (, int96 flowRate, , ) = cs.cfaV1.cfa.getFlow(
-            paymentToken,
-            oldCurrentBid.bidder,
-            address(this)
-        );
-        if (flowRate > 0) {
-            cs.cfaV1.deleteFlow(
-                oldCurrentBid.bidder,
-                address(this),
-                paymentToken
-            );
-        }
-
         (int256 availableBalance, uint256 deposit, , ) = paymentToken
             .realtimeBalanceOfNow(address(this));
         uint256 remainingBalance = 0;
@@ -156,7 +142,7 @@ library LibCFAPenaltyBid {
         }
 
         // Update beneficiary flow
-        (, flowRate, , ) = cs.cfaV1.cfa.getFlow(
+        (, int96 flowRate, , ) = cs.cfaV1.cfa.getFlow(
             paymentToken,
             address(this),
             beneficiary
@@ -171,41 +157,60 @@ library LibCFAPenaltyBid {
             LibCFABasePCO._createBeneficiaryFlow(_pendingBid.contributionRate);
         }
 
-        // Transfer license
-        ds.license.safeTransferFrom(
-            oldCurrentBid.bidder,
-            _pendingBid.bidder,
-            ds.licenseId
-        );
-
-        // Transfer payments
-        uint256 withdrawToBidder = 0;
-        uint256 withdrawToPayer = 0;
-
-        if (remainingBalance > newBuffer) {
-            // Keep full newBuffer
-            remainingBalance -= newBuffer;
-            uint256 bidderPayment = _pendingBid.forSalePrice -
-                oldCurrentBid.forSalePrice;
-            if (remainingBalance > bidderPayment) {
-                // Transfer bidder full payment
-                withdrawToBidder = bidderPayment;
-                remainingBalance -= withdrawToBidder;
+        {
+            // Transfer payments
+            uint256 withdrawToBidder = 0;
+            uint256 withdrawToPayer = 0;
+
+            if (remainingBalance > newBuffer) {
+                // Keep full newBuffer
+                remainingBalance -= newBuffer;
+                uint256 bidderPayment = _pendingBid.forSalePrice -
+                    oldCurrentBid.forSalePrice;
+                if (remainingBalance > bidderPayment) {
+                    // Transfer bidder full payment
+                    withdrawToBidder = bidderPayment;
+                    remainingBalance -= withdrawToBidder;
+
+                    // Transfer remaining to payer
+                    withdrawToPayer = remainingBalance;
+                } else {
+                    // Transfer remaining to bidder
+                    withdrawToBidder = remainingBalance;
+                }
+            }
 
-                // Transfer remaining to payer
-                withdrawToPayer = remainingBalance;
-            } else {
-                // Transfer remaining to bidder
-                withdrawToBidder = remainingBalance;
+            if (withdrawToBidder > 0) {
+                paymentToken.safeTransfer(_pendingBid.bidder, withdrawToBidder);
+            }
+            if (withdrawToPayer > 0) {
+                paymentToken.safeTransfer(
+                    oldCurrentBid.bidder,
+                    withdrawToPayer
+                );
             }
         }
 
-        if (withdrawToBidder > 0) {
-            paymentToken.safeTransfer(_pendingBid.bidder, withdrawToBidder);
-        }
-        if (withdrawToPayer > 0) {
-            paymentToken.safeTransfer(oldCurrentBid.bidder, withdrawToPayer);
+        // Delete payer flow (reentrancy on potential SuperApp callback)
+        (, flowRate, , ) = cs.cfaV1.cfa.getFlow(
+            paymentToken,
+            oldCurrentBid.bidder,
+            address(this)
+        );
+        if (flowRate > 0) {
+            cs.cfaV1.deleteFlow(
+                oldCurrentBid.bidder,
+                address(this),
+                paymentToken
+            );
         }
+
+        // Transfer license (reentrancy on ERC721 transfer)
+        ds.license.safeTransferFrom(
+            oldCurrentBid.bidder,
+            _pendingBid.bidder,
+            ds.licenseId
+        );
     }
 
     /// @notice Reject Bid

diff --git a/contracts/registry/facets/PCOLicenseClaimerFacet.sol b/contracts/registry/facets/PCOLicenseClaimerFacet.sol
@@ -241,9 +241,6 @@ contract PCOLicenseClaimerFacetV1 is IPCOLicenseClaimerV1, ERC721BaseInternal {
 
         emit ParcelClaimed(licenseId, msg.sender);
 
-        // Build and mint
-        _buildAndMint(msg.sender, baseCoordinate, path);
-
         {
             // Transfer required buffer
             IConstantFlowAgreementV1 cfa = IConstantFlowAgreementV1(
@@ -285,6 +282,9 @@ contract PCOLicenseClaimerFacetV1 is IPCOLicenseClaimerV1, ERC721BaseInternal {
                 _requiredBid
             );
         }
+
+        // Build and mint (reentrancy on ERC721 transfer)
+        _buildAndMint(msg.sender, baseCoordinate, path);
     }
 
     /**
@@ -350,9 +350,6 @@ contract PCOLicenseClaimerFacetV2 is
 
         emit ParcelClaimedV2(licenseId, msg.sender);
 
-        // Build and mint
-        _buildAndMint(msg.sender, parcel);
-
         {
             // Transfer required buffer
             IConstantFlowAgreementV1 cfa = IConstantFlowAgreementV1(
@@ -394,6 +391,9 @@ contract PCOLicenseClaimerFacetV2 is
                 _requiredBid
             );
         }
+
+        // Build and mint (reentrancy on ERC721 transfer)
+        _buildAndMint(msg.sender, parcel);
     }
 
     /**

diff --git a/test/registry/PCOLicenseClaimerFacet.ts b/test/registry/PCOLicenseClaimerFacet.ts
@@ -534,6 +534,13 @@ describe("PCOLicenseClaimerFacet", async function () {
         .connect(await ethers.getSigner(user))
         .claim(contributionRate, forSalePrice, coord, [BigNumber.from(0)]);
 
+      // Approve payment token for buffer
+      const approveOp1 = paymentToken.approve({
+        receiver: pcoLicenseClaimer.address,
+        amount: requiredBuffer.toString(),
+      });
+      await approveOp1.exec(await ethers.getSigner(user));
+
       const txn = pcoLicenseClaimer
         .connect(await ethers.getSigner(user))
         .claim(contributionRate, forSalePrice, coord, [BigNumber.from(0)]);

diff --git a/test/registry/PCOLicenseClaimerFacetV2.ts b/test/registry/PCOLicenseClaimerFacetV2.ts
@@ -620,6 +620,12 @@ describe("PCOLicenseClaimerFacetV2", async function () {
           }
         );
 
+      const approveOp1 = paymentToken.approve({
+        receiver: pcoLicenseClaimer.address,
+        amount: requiredBuffer.toString(),
+      });
+      await approveOp1.exec(await ethers.getSigner(user));
+
       const txn = pcoLicenseClaimerV2
         .connect(await ethers.getSigner(user))
         ["claim(int96,uint256,(uint64,uint256,uint256))"](

diff --git a/typechain-types/factories/CFAPenaltyBidFacet__factory.ts b/typechain-types/factories/CFAPenaltyBidFacet__factory.ts