Remediate returnbomb during a twin transfer (#772)

* handle returnbomb

* returnbomb in ERC721 and ERC1155

contracts/mock/Foreign1155.sol

@@ -39,6 +39,21 @@ contract Foreign1155GasTheft is Foreign1155 {
     }
 }
 
+/*
+ * @title Foreign1155 that returns an absurdly long return data
+ *
+ * @notice Mock ERC-(1155) for Unit Testing
+ */
+contract Foreign1155ReturnBomb is Foreign1155 {
+    function safeTransferFrom(address, address, uint256, uint256, bytes memory) public virtual override {
+        assembly {
+            revert(0, 3000000)
+            // This is carefully chosen. If it's too low, not enough gas is consumed and contract that call it does not run out of gas.
+            // If it's too high, then this contract runs out of gas before the return data is returned.
+        }
+    }
+}
+
 /*
  * @title Foreign1155 that succeeds, but the data cannot be decoded into a boolean
  *

contracts/mock/Foreign721.sol

@@ -45,6 +45,21 @@ contract Foreign721GasTheft is Foreign721 {
     }
 }
 
+/*
+ * @title Foreign721 that returns an absurdly long return data
+ *
+ * @notice Mock ERC-(721) for Unit Testing
+ */
+contract Foreign721ReturnBomb is Foreign721 {
+    function safeTransferFrom(address, address, uint256, bytes memory) public virtual override {
+        assembly {
+            revert(0, 3000000)
+            // This is carefully chosen. If it's too low, not enough gas is consumed and contract that call it does not run out of gas.
+            // If it's too high, then this contract runs out of gas before the return data is returned.
+        }
+    }
+}
+
 /*
  * @title Foreign721 that succeeds, but the data cannot be decoded into a boolean
  *

contracts/protocol/facets/ExchangeHandlerFacet.sol

@@ -894,9 +894,35 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase {
                     // Make call only if there is enough gas and code at address exists.
                     // If not, skip the call and mark the transfer as failed
                     twinM.tokenAddress = twinS.tokenAddress;
-                    if (gasleft() > reservedGas && twinM.tokenAddress.isContract()) {
+                    uint256 gasLeft = gasleft();
+                    if (gasLeft > reservedGas && twinM.tokenAddress.isContract()) {
+                        address to = twinM.tokenAddress;
+
+                        // Handle the return value with assembly to avoid return bomb attack
                         bytes memory result;
-                        (success, result) = twinM.tokenAddress.call{ gas: gasleft() - reservedGas }(data);
+                        assembly {
+                            success := call(
+                                sub(gasLeft, reservedGas), // gasleft()-reservedGas
+                                to, // twin contract
+                                0, // ether value
+                                add(data, 0x20), // invocation calldata
+                                mload(data), // calldata length
+                                add(result, 0x20), // store return data at result
+                                0x20 // store at most 32 bytes
+                            )
+
+                            let returndataSize := returndatasize()
+
+                            switch gt(returndataSize, 0x20)
+                            case 0 {
+                                // Adjust result length in case it's shorter than 32 bytes
+                                mstore(result, returndataSize)
+                            }
+                            case 1 {
+                                // If return data is longer than 32 bytes, consider transfer unsuccesful
+                                success := false
+                            }
+                        }
 
                         // Check if result is empty or if result is a boolean and is true
                         success =

test/protocol/ExchangeHandlerTest.js

@@ -3621,6 +3621,52 @@ describe("IBosonExchangeHandler", function () {
             assert.equal(response, ExchangeState.Disputed, "Exchange state is incorrect");
           });
 
+          it("if twin returns a long return, redeem still succeeds, but exchange is disputed", async function () {
+            const [foreign20rb] = await deployMockTokens(["Foreign20ReturnBomb"]);
+
+            // Approve the protocol diamond to transfer seller's tokens
+            await foreign20rb.connect(assistant).approve(protocolDiamondAddress, "100");
+
+            // Create two ERC20 twins that will consume all available gas
+            twin20 = mockTwin(await foreign20rb.getAddress());
+            twin20.amount = "1";
+            twin20.supplyAvailable = "100";
+            twin20.id = "4";
+
+            await twinHandler.connect(assistant).createTwin(twin20.toStruct());
+
+            // Create a new offer and bundle
+            await offerHandler
+              .connect(assistant)
+              .createOffer(offer, offerDates, offerDurations, disputeResolverId, agentId);
+            bundle = new Bundle("2", seller.id, [`${++offerId}`], [twin20.id]);
+            await bundleHandler.connect(assistant).createBundle(bundle.toStruct());
+
+            // Commit to offer
+            const buyerAddress = await buyer.getAddress();
+            await exchangeHandler.connect(buyer).commitToOffer(buyerAddress, offerId, { value: price });
+
+            exchange.id = Number(exchange.id) + 1;
+
+            // Redeem the voucher
+            tx = await exchangeHandler.connect(buyer).redeemVoucher(exchange.id);
+
+            // Dispute should be raised and both transfers should fail
+            await expect(tx)
+              .to.emit(disputeHandler, "DisputeRaised")
+              .withArgs(exchange.id, exchange.buyerId, seller.id, buyerAddress);
+
+            await expect(tx)
+              .to.emit(exchangeHandler, "TwinTransferFailed")
+              .withArgs(twin20.id, twin20.tokenAddress, exchange.id, twin20.tokenId, twin20.amount, buyerAddress);
+
+            // Get the exchange state
+            [, response] = await exchangeHandler.connect(rando).getExchangeState(exchange.id);
+
+            // It should match ExchangeState.Disputed
+            assert.equal(response, ExchangeState.Disputed, "Exchange state is incorrect");
+          });
+
           context("Malformed return", async function () {
             const attackTypes = {
               "too short": 0,
@@ -4278,6 +4324,53 @@ describe("IBosonExchangeHandler", function () {
             assert.equal(response, ExchangeState.Disputed, "Exchange state is incorrect");
           });
 
+          it("if twin returns a long return, redeem still succeeds, but exchange is disputed", async function () {
+            const [foreign721rb] = await deployMockTokens(["Foreign721ReturnBomb"]);
+
+            // Approve the protocol diamond to transfer seller's tokens
+            await foreign721rb.connect(assistant).setApprovalForAll(protocolDiamondAddress, true);
+
+            // Create two ERC721 twins that will consume all available gas
+            twin721 = mockTwin(await foreign721rb.getAddress(), TokenType.NonFungibleToken);
+            twin721.amount = "0";
+            twin721.supplyAvailable = "10";
+            twin721.id = "4";
+
+            await twinHandler.connect(assistant).createTwin(twin721.toStruct());
+
+            // Create a new offer and bundle
+            await offerHandler
+              .connect(assistant)
+              .createOffer(offer, offerDates, offerDurations, disputeResolverId, agentId);
+            bundle = new Bundle("2", seller.id, [`${++offerId}`], [twin721.id]);
+            await bundleHandler.connect(assistant).createBundle(bundle.toStruct());
+
+            // Commit to offer
+            const buyerAddress = await buyer.getAddress();
+            await exchangeHandler.connect(buyer).commitToOffer(buyerAddress, offerId, { value: price });
+
+            exchange.id = Number(exchange.id) + 1;
+
+            // Redeem the voucher
+            tx = await exchangeHandler.connect(buyer).redeemVoucher(exchange.id);
+
+            // Dispute should be raised and both transfers should fail
+            await expect(tx)
+              .to.emit(disputeHandler, "DisputeRaised")
+              .withArgs(exchange.id, exchange.buyerId, seller.id, buyerAddress);
+
+            let tokenId = "9";
+            await expect(tx)
+              .to.emit(exchangeHandler, "TwinTransferFailed")
+              .withArgs(twin721.id, twin721.tokenAddress, exchange.id, tokenId, twin721.amount, buyerAddress);
+
+            // Get the exchange state
+            [, response] = await exchangeHandler.connect(rando).getExchangeState(exchange.id);
+
+            // It should match ExchangeState.Disputed
+            assert.equal(response, ExchangeState.Disputed, "Exchange state is incorrect");
+          });
+
           context("Malformed return", async function () {
             const attackTypes = {
               "too short": 0,
@@ -4627,6 +4720,60 @@ describe("IBosonExchangeHandler", function () {
             assert.equal(response, ExchangeState.Disputed, "Exchange state is incorrect");
           });
 
+          it("if twin returns a long return, redeem still succeeds, but exchange is disputed", async function () {
+            const [foreign1155rb] = await deployMockTokens(["Foreign1155ReturnBomb"]);
+
+            // Approve the protocol diamond to transfer seller's tokens
+            await foreign1155rb.connect(assistant).setApprovalForAll(protocolDiamondAddress, true);
+
+            // Create two ERC1155 twins that will consume all available gas
+            twin1155 = mockTwin(await foreign1155rb.getAddress(), TokenType.MultiToken);
+            twin1155.amount = "1";
+            twin1155.tokenId = "1";
+            twin1155.supplyAvailable = "10";
+            twin1155.id = "4";
+
+            await twinHandler.connect(assistant).createTwin(twin1155.toStruct());
+
+            // Create a new offer and bundle
+            await offerHandler
+              .connect(assistant)
+              .createOffer(offer, offerDates, offerDurations, disputeResolverId, agentId);
+            bundle = new Bundle("2", seller.id, [`${++offerId}`], [twin1155.id]);
+            await bundleHandler.connect(assistant).createBundle(bundle.toStruct());
+
+            // Commit to offer
+            const buyerAddress = await buyer.getAddress();
+            await exchangeHandler.connect(buyer).commitToOffer(buyerAddress, offerId, { value: price });
+
+            exchange.id = Number(exchange.id) + 1;
+
+            // Redeem the voucher
+            tx = await exchangeHandler.connect(buyer).redeemVoucher(exchange.id);
+
+            // Dispute should be raised and both transfers should fail
+            await expect(tx)
+              .to.emit(disputeHandler, "DisputeRaised")
+              .withArgs(exchange.id, exchange.buyerId, seller.id, buyerAddress);
+
+            await expect(tx)
+              .to.emit(exchangeHandler, "TwinTransferFailed")
+              .withArgs(
+                twin1155.id,
+                twin1155.tokenAddress,
+                exchange.id,
+                twin1155.tokenId,
+                twin1155.amount,
+                buyerAddress
+              );
+
+            // Get the exchange state
+            [, response] = await exchangeHandler.connect(rando).getExchangeState(exchange.id);
+
+            // It should match ExchangeState.Disputed
+            assert.equal(response, ExchangeState.Disputed, "Exchange state is incorrect");
+          });
+
           context("Malformed return", async function () {
             const attackTypes = {
               "too short": 0,