diff --git a/contracts/mock/Foreign1155.sol b/contracts/mock/Foreign1155.sol index 4717a0fba..24f527ce1 100644 --- a/contracts/mock/Foreign1155.sol +++ b/contracts/mock/Foreign1155.sol @@ -38,3 +38,38 @@ contract Foreign1155GasTheft is Foreign1155 { } } } + +/* + * @title Foreign1155 that succeeds, but the data cannot be decoded into a boolean + * + * @notice Mock ERC-(1155) for Unit Testing + */ +contract Foreign1155MalformedReturn is Foreign1155 { + enum AttackType { + ReturnTooShort, + ReturnTooLong, + ReturnInvalid + } + + AttackType public attackType; + + function setAttackType(AttackType _attackType) external { + attackType = _attackType; + } + + function safeTransferFrom(address, address, uint256, uint256, bytes memory) public virtual override { + if (attackType == AttackType.ReturnTooShort) { + assembly { + return(0, 31) // return too short data + } + } else if (attackType == AttackType.ReturnTooLong) { + assembly { + return(0, 33) // return too long data + } + } else if (attackType == AttackType.ReturnInvalid) { + assembly { + return(0x40, 32) // return a value that is not 0 or 1 + } + } + } +} diff --git a/contracts/mock/Foreign20.sol b/contracts/mock/Foreign20.sol index 5db0439f8..147990c2e 100644 --- a/contracts/mock/Foreign20.sol +++ b/contracts/mock/Foreign20.sol @@ -246,3 +246,53 @@ contract Foreign20GasTheft is Foreign20 { return false; } } + +/* + * @title Foreign20 that returns an absurdly long return data + * + * @notice Mock ERC-(20) for Unit Testing + */ +contract Foreign20ReturnBomb is Foreign20 { + function transferFrom(address, address, uint256) public virtual override returns (bool) { + 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 Foreign20 that succeeds, but the data cannot be decoded into a boolean + * + * @notice Mock ERC-(20) for Unit Testing + */ +contract Foreign20MalformedReturn is Foreign20 { + enum AttackType { + ReturnTooShort, + ReturnTooLong, + ReturnInvalid + } + + AttackType public attackType; + + function setAttackType(AttackType _attackType) external { + attackType = _attackType; + } + + function transferFrom(address, address, uint256) public virtual override returns (bool) { + if (attackType == AttackType.ReturnTooShort) { + assembly { + return(0, 31) // return too short data + } + } else if (attackType == AttackType.ReturnTooLong) { + assembly { + return(0, 33) // return too long data + } + } else if (attackType == AttackType.ReturnInvalid) { + assembly { + return(0x40, 32) // return a value that is not 0 or 1 + } + } + } +} diff --git a/contracts/mock/Foreign721.sol b/contracts/mock/Foreign721.sol index d289d70e3..64f04d883 100644 --- a/contracts/mock/Foreign721.sol +++ b/contracts/mock/Foreign721.sol @@ -44,3 +44,38 @@ contract Foreign721GasTheft is Foreign721 { } } } + +/* + * @title Foreign721 that succeeds, but the data cannot be decoded into a boolean + * + * @notice Mock ERC-(721) for Unit Testing + */ +contract Foreign721MalformedReturn is Foreign721 { + enum AttackType { + ReturnTooShort, + ReturnTooLong, + ReturnInvalid + } + + AttackType public attackType; + + function setAttackType(AttackType _attackType) external { + attackType = _attackType; + } + + function safeTransferFrom(address, address, uint256, bytes memory) public virtual override { + if (attackType == AttackType.ReturnTooShort) { + assembly { + return(0, 31) // return too short data + } + } else if (attackType == AttackType.ReturnTooLong) { + assembly { + return(0, 33) // return too long data + } + } else if (attackType == AttackType.ReturnInvalid) { + assembly { + return(0x40, 32) // return a value that is not 0 or 1 + } + } + } +} diff --git a/contracts/protocol/facets/ExchangeHandlerFacet.sol b/contracts/protocol/facets/ExchangeHandlerFacet.sol index a04473f1a..054419b72 100644 --- a/contracts/protocol/facets/ExchangeHandlerFacet.sol +++ b/contracts/protocol/facets/ExchangeHandlerFacet.sol @@ -898,7 +898,10 @@ contract ExchangeHandlerFacet is IBosonExchangeHandler, BuyerBase, DisputeBase { bytes memory result; (success, result) = twinM.tokenAddress.call{ gas: gasleft() - reservedGas }(data); - success = success && (result.length == 0 || abi.decode(result, (bool))); + // Check if result is empty or if result is a boolean and is true + success = + success && + (result.length == 0 || (result.length == 32 && abi.decode(result, (uint256)) == 1)); } } diff --git a/test/protocol/ExchangeHandlerTest.js b/test/protocol/ExchangeHandlerTest.js index 70d507c03..e5d36d593 100644 --- a/test/protocol/ExchangeHandlerTest.js +++ b/test/protocol/ExchangeHandlerTest.js @@ -3555,11 +3555,11 @@ describe("IBosonExchangeHandler", function () { // Get the exchange state [, response] = await exchangeHandler.connect(rando).getExchangeState(exchange.id); - // It should match ExchangeState.Revoked + // It should match ExchangeState.Disputed assert.equal(response, ExchangeState.Disputed, "Exchange state is incorrect"); }); - it("if twin transfers consume all available gas, redeem still succeeds, but exchange is revoked", async function () { + it("if twin transfers consume all available gas, redeem still succeeds, but exchange is disputed", async function () { const [foreign20gt, foreign20gt_2] = await deployMockTokens(["Foreign20GasTheft", "Foreign20GasTheft"]); // Approve the protocol diamond to transfer seller's tokens @@ -3595,7 +3595,7 @@ describe("IBosonExchangeHandler", function () { // Redeem the voucher tx = await exchangeHandler.connect(buyer).redeemVoucher(exchange.id, { gasLimit: 1000000 }); // limit gas to speed up test - // Voucher should be revoked and both transfers should fail + // Dispute should be raised and both transfers should fail await expect(tx) .to.emit(disputeHandler, "DisputeRaised") .withArgs(exchange.id, exchange.buyerId, seller.id, buyerAddress); @@ -3621,6 +3621,67 @@ describe("IBosonExchangeHandler", function () { assert.equal(response, ExchangeState.Disputed, "Exchange state is incorrect"); }); + context("Malformed return", async function () { + const attackTypes = { + "too short": 0, + "too long": 1, + invalid: 2, + }; + let foreign20mr; + + beforeEach(async function () { + [foreign20mr] = await deployMockTokens(["Foreign20MalformedReturn"]); + + // Approve the protocol diamond to transfer seller's tokens + await foreign20mr.connect(assistant).approve(protocolDiamondAddress, "100"); + + // Create two ERC20 twins that will consume all available gas + twin20 = mockTwin(await foreign20mr.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 + await exchangeHandler.connect(buyer).commitToOffer(buyer.address, offerId, { value: price }); + + exchange.id = Number(exchange.id) + 1; + }); + + Object.entries(attackTypes).forEach((attackType) => { + const [type, enumType] = attackType; + it(`return value is ${type}, redeem still succeeds, but the exchange is disputed`, async function () { + await foreign20mr.setAttackType(enumType); + + // 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, buyer.address); + + await expect(tx) + .to.emit(exchangeHandler, "TwinTransferFailed") + .withArgs(twin20.id, twin20.tokenAddress, exchange.id, twin20.tokenId, twin20.amount, buyer.address); + + // 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"); + }); + }); + }); + it("If multiple transfers fail, a dispute is raised only once", async function () { const [foreign20, foreign20_2] = await deployMockTokens(["Foreign20", "Foreign20"]); @@ -4156,11 +4217,11 @@ describe("IBosonExchangeHandler", function () { // Get the exchange state [, response] = await exchangeHandler.connect(rando).getExchangeState(exchange.id); - // It should match ExchangeState.Revoked + // It should match ExchangeState.Disputed assert.equal(response, ExchangeState.Disputed, "Exchange state is incorrect"); }); - it("if twin transfers consume all available gas, redeem still succeeds, but exchange is revoked", async function () { + it("if twin transfers consume all available gas, redeem still succeeds, but exchange is disputed", async function () { const [foreign721gt, foreign721gt_2] = await deployMockTokens(["Foreign721GasTheft", "Foreign721GasTheft"]); // Approve the protocol diamond to transfer seller's tokens @@ -4196,7 +4257,7 @@ describe("IBosonExchangeHandler", function () { // Redeem the voucher tx = await exchangeHandler.connect(buyer).redeemVoucher(exchange.id, { gasLimit: 1000000 }); // limit gas to speed up test - // Voucher should be revoked and both transfers should fail + // Dispute should be raised and both transfers should fail await expect(tx) .to.emit(disputeHandler, "DisputeRaised") .withArgs(exchange.id, exchange.buyerId, seller.id, buyerAddress); @@ -4217,6 +4278,68 @@ describe("IBosonExchangeHandler", function () { assert.equal(response, ExchangeState.Disputed, "Exchange state is incorrect"); }); + context("Malformed return", async function () { + const attackTypes = { + "too short": 0, + "too long": 1, + invalid: 2, + }; + let foreign721mr; + + beforeEach(async function () { + [foreign721mr] = await deployMockTokens(["Foreign721MalformedReturn"]); + + // Approve the protocol diamond to transfer seller's tokens + await foreign721mr.connect(assistant).setApprovalForAll(protocolDiamondAddress, true); + + // Create two ERC721 twins that will consume all available gas + twin721 = mockTwin(await foreign721mr.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 + await exchangeHandler.connect(buyer).commitToOffer(buyer.address, offerId, { value: price }); + + exchange.id = Number(exchange.id) + 1; + }); + + Object.entries(attackTypes).forEach((attackType) => { + const [type, enumType] = attackType; + it(`return value is ${type}, redeem still succeeds, but the exchange is disputed`, async function () { + await foreign721mr.setAttackType(enumType); + + // 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, buyer.address); + + let tokenId = "9"; + await expect(tx) + .to.emit(exchangeHandler, "TwinTransferFailed") + .withArgs(twin721.id, twin721.tokenAddress, exchange.id, tokenId, twin721.amount, buyer.address); + + // 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"); + }); + }); + }); + it("should raise a dispute if erc721 contract does not exist anymore", async function () { // Destruct the ERC721 await foreign721.destruct(); @@ -4426,11 +4549,11 @@ describe("IBosonExchangeHandler", function () { // Get the exchange state [, response] = await exchangeHandler.connect(rando).getExchangeState(exchange.id); - // It should match ExchangeState.Revoked + // It should match ExchangeState.Disputed assert.equal(response, ExchangeState.Disputed, "Exchange state is incorrect"); }); - it("if twin transfers consume all available gas, redeem still succeeds, but exchange is revoked", async function () { + it("if twin transfers consume all available gas, redeem still succeeds, but exchange is disputed", async function () { const [foreign1155gt, foreign1155gt_2] = await deployMockTokens([ "Foreign1155GasTheft", "Foreign1155GasTheft", @@ -4470,6 +4593,11 @@ describe("IBosonExchangeHandler", function () { // Redeem the voucher tx = await exchangeHandler.connect(buyer).redeemVoucher(exchange.id, { gasLimit: 1000000 }); // limit gas to speed up test + // Dispute should be raised and both transfers should fail + await expect(tx) + .to.emit(disputeHandler, "DisputeRaised") + .withArgs(exchange.id, exchange.buyerId, seller.id, buyer.address); + await expect(tx) .to.emit(exchangeHandler, "TwinTransferFailed") .withArgs( @@ -4499,6 +4627,75 @@ describe("IBosonExchangeHandler", function () { assert.equal(response, ExchangeState.Disputed, "Exchange state is incorrect"); }); + context("Malformed return", async function () { + const attackTypes = { + "too short": 0, + "too long": 1, + invalid: 2, + }; + let foreign1155mr; + + beforeEach(async function () { + [foreign1155mr] = await deployMockTokens(["Foreign1155MalformedReturn"]); + + // Approve the protocol diamond to transfer seller's tokens + await foreign1155mr.connect(assistant).setApprovalForAll(protocolDiamondAddress, true); + + // Create two ERC1155 twins that will consume all available gas + twin1155 = mockTwin(await foreign1155mr.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 + await exchangeHandler.connect(buyer).commitToOffer(buyer.address, offerId, { value: price }); + + exchange.id = Number(exchange.id) + 1; + }); + + Object.entries(attackTypes).forEach((attackType) => { + const [type, enumType] = attackType; + it(`return value is ${type}, redeem still succeeds, but the exchange is disputed`, async function () { + await foreign1155mr.setAttackType(enumType); + + // 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, buyer.address); + + await expect(tx) + .to.emit(exchangeHandler, "TwinTransferFailed") + .withArgs( + twin1155.id, + twin1155.tokenAddress, + exchange.id, + twin1155.tokenId, + twin1155.amount, + buyer.address + ); + + // 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"); + }); + }); + }); + it("should raise a dispute if erc1155 contract does not exist anymore", async function () { // Destruct the ERC1155 contract await foreign1155.destruct(); @@ -4915,11 +5112,11 @@ describe("IBosonExchangeHandler", function () { // Get the exchange state [, response] = await exchangeHandler.connect(rando).getExchangeState(exchange.id); - // It should match ExchangeState.Revoked + // It should match ExchangeState.Disputed assert.equal(response, ExchangeState.Disputed, "Exchange state is incorrect"); }); - it("if twin transfers consume all available gas, redeem still succeeds, but exchange is revoked", async function () { + it("if twin transfers consume all available gas, redeem still succeeds, but exchange is disputed", async function () { const [foreign20gt, foreign721gt, foreign1155gt] = await deployMockTokens([ "Foreign20GasTheft", "Foreign721GasTheft", @@ -4968,7 +5165,7 @@ describe("IBosonExchangeHandler", function () { // Redeem the voucher tx = await exchangeHandler.connect(buyer).redeemVoucher(exchange.id, { gasLimit: 1000000 }); // limit gas to speed up test - // Voucher should be revoked and both transfers should fail + // Dispute should be raised and both transfers should fail await expect(tx) .to.emit(disputeHandler, "DisputeRaised") .withArgs(exchange.id, exchange.buyerId, seller.id, buyerAddress);