diff --git a/contracts/mock/Foreign1155.sol b/contracts/mock/Foreign1155.sol index 3cdbbdd7f..3c4389bd2 100644 --- a/contracts/mock/Foreign1155.sol +++ b/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 * diff --git a/contracts/mock/Foreign721.sol b/contracts/mock/Foreign721.sol index 9483e7d70..82853ad4d 100644 --- a/contracts/mock/Foreign721.sol +++ b/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 * diff --git a/contracts/protocol/facets/ExchangeHandlerFacet.sol b/contracts/protocol/facets/ExchangeHandlerFacet.sol index 93313c822..fc19867d9 100644 --- a/contracts/protocol/facets/ExchangeHandlerFacet.sol +++ b/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 = diff --git a/test/protocol/ExchangeHandlerTest.js b/test/protocol/ExchangeHandlerTest.js index e5d36d593..10c5e8aa4 100644 --- a/test/protocol/ExchangeHandlerTest.js +++ b/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,