From d09d39f238ad05c58ab882c51847d8949330e1c3 Mon Sep 17 00:00:00 2001 From: byshape Date: Thu, 1 Aug 2024 13:42:14 +0100 Subject: [PATCH 1/2] Forbid to fill the same part of an order more than once --- .gas-snapshot | 34 +++++++++---------- contracts/BaseEscrowFactory.sol | 7 ++++ contracts/MerkleStorageInvalidator.sol | 1 - contracts/interfaces/IEscrowFactory.sol | 1 + .../interfaces/IMerkleStorageInvalidator.sol | 1 - .../MerkleStorageInvalidator.t.sol | 30 ++++++++++++++-- 6 files changed, 53 insertions(+), 21 deletions(-) diff --git a/.gas-snapshot b/.gas-snapshot index 363a35c..b1d9e2c 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -1,4 +1,4 @@ -EscrowFactoryTest:test_MultipleFillsInvalidKey() (gas: 474203) +EscrowFactoryTest:test_MultipleFillsInvalidKey() (gas: 474261) EscrowFactoryTest:test_MultipleFillsInvalidSecretsAmount() (gas: 473799) EscrowFactoryTest:test_NoDeploymentForNotResolver() (gas: 190767) EscrowFactoryTest:test_NoInsufficientBalanceDeploymentForMaker() (gas: 128468) @@ -12,7 +12,7 @@ EscrowTest:test_CancelDstWithNativeToken() (gas: 93622) EscrowTest:test_CancelPublicSrc() (gas: 165457) EscrowTest:test_CancelResolverSrc() (gas: 168586) EscrowTest:test_CancelResolverSrcReceiver() (gas: 179340) -EscrowTest:test_NoAnyoneCancelDuringResolverCancelSrc() (gas: 163829) +EscrowTest:test_NoAnyoneCancelDuringResolverCancelSrc() (gas: 163809) EscrowTest:test_NoCallsWithInvalidImmutables() (gas: 286454) EscrowTest:test_NoCancelByAnyoneDst() (gas: 121678) EscrowTest:test_NoCancelDuringWithdrawalDst() (gas: 121438) @@ -20,7 +20,7 @@ EscrowTest:test_NoCancelDuringWithdrawalSrc() (gas: 163980) EscrowTest:test_NoFailedNativeTokenTransferCancelSrc() (gas: 179317) EscrowTest:test_NoFailedNativeTokenTransferWithdrawalDst() (gas: 154458) EscrowTest:test_NoFailedNativeTokenTransferWithdrawalDstNative() (gas: 83365) -EscrowTest:test_NoFailedNativeTokenTransferWithdrawalSrc() (gas: 312353) +EscrowTest:test_NoFailedNativeTokenTransferWithdrawalSrc() (gas: 312313) EscrowTest:test_NoPublicCancelDuringPrivateCancellationSrc() (gas: 163124) EscrowTest:test_NoPublicWithdrawOutsideOfAllowedPeriodDst() (gas: 126855) EscrowTest:test_NoPublicWithdrawalOutsideOfAllowedPeriodSrc() (gas: 169069) @@ -37,13 +37,13 @@ EscrowTest:test_NoWithdrawalWithWrongSecretSrc() (gas: 164438) EscrowTest:test_PublicWithdrawSrc() (gas: 181729) EscrowTest:test_RescueFundsDst() (gas: 158179) EscrowTest:test_RescueFundsDstNative() (gas: 186642) -EscrowTest:test_RescueFundsSrc() (gas: 195453) -EscrowTest:test_RescueFundsSrcNative() (gas: 197720) +EscrowTest:test_RescueFundsSrc() (gas: 195445) +EscrowTest:test_RescueFundsSrcNative() (gas: 197721) EscrowTest:test_WithdrawByAnyoneDst() (gas: 141240) EscrowTest:test_WithdrawByResolverDst() (gas: 142325) EscrowTest:test_WithdrawByResolverDstNative() (gas: 97810) EscrowTest:test_WithdrawByResolverPublicDst() (gas: 141671) -EscrowTest:test_WithdrawSrc() (gas: 186512) +EscrowTest:test_WithdrawSrc() (gas: 186495) EscrowTest:test_WithdrawSrcTo() (gas: 191326) IntegrationEscrowFactoryTest:test_DeployCloneForMakerNonWhitelistedResolverInt() (gas: 473480) IntegrationEscrowFactoryTest:test_NoInsufficientBalanceDeploymentForMakerInt() (gas: 341110) @@ -57,17 +57,17 @@ IntegrationResolverMockTest:test_MockRescueFundsDst() (gas: 161014) IntegrationResolverMockTest:test_MockRescueFundsSrc() (gas: 382547) IntegrationResolverMockTest:test_MockWithdrawDst() (gas: 182848) IntegrationResolverMockTest:test_MockWithdrawToSrc() (gas: 354840) -MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllExtra() (gas: 923993) -MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllTwoFills() (gas: 922676) -MerkleStorageInvalidatorIntTest:test_MultipleFillsFillFirst() (gas: 707799) -MerkleStorageInvalidatorIntTest:test_MultipleFillsFillFirstTwoFills() (gas: 933042) -MerkleStorageInvalidatorIntTest:test_MultipleFillsFillLast() (gas: 707543) -MerkleStorageInvalidatorIntTest:test_MultipleFillsNoDeploymentWithoutValidation() (gas: 301349) -MerkleStorageInvalidatorIntTest:test_MultipleFillsNoReuseOfSecrets() (gas: 1002770) -MerkleStorageInvalidatorIntTest:test_MultipleFillsNoSecondDeploymentWithTheSameIndex() (gas: 778560) -MerkleStorageInvalidatorIntTest:test_MultipleFillsOddDivision() (gas: 443533) -MerkleStorageInvalidatorIntTest:test_MultipleFillsOneFill() (gas: 707769) -MerkleStorageInvalidatorIntTest:test_MultipleFillsTwoFills() (gas: 921506) +MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllExtra() (gas: 923753) +MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllTwoFills() (gas: 922423) +MerkleStorageInvalidatorIntTest:test_MultipleFillsFillFirst() (gas: 707679) +MerkleStorageInvalidatorIntTest:test_MultipleFillsFillFirstTwoFills() (gas: 933077) +MerkleStorageInvalidatorIntTest:test_MultipleFillsFillLast() (gas: 707393) +MerkleStorageInvalidatorIntTest:test_MultipleFillsNoDeploymentWithoutValidation() (gas: 301520) +MerkleStorageInvalidatorIntTest:test_MultipleFillsNoReuseOfSecrets() (gas: 1052445) +MerkleStorageInvalidatorIntTest:test_MultipleFillsNoSecondDeploymentWithTheSameIndex() (gas: 786004) +MerkleStorageInvalidatorIntTest:test_MultipleFillsOddDivision() (gas: 443474) +MerkleStorageInvalidatorIntTest:test_MultipleFillsOneFill() (gas: 707716) +MerkleStorageInvalidatorIntTest:test_MultipleFillsTwoFills() (gas: 921543) TimelocksLibTest:test_NoTimelocksOverflow() (gas: 133743) TimelocksLibTest:test_getStartTimestamps() (gas: 16180) TimelocksLibTest:test_setDeployedAt() (gas: 5731) \ No newline at end of file diff --git a/contracts/BaseEscrowFactory.sol b/contracts/BaseEscrowFactory.sol index aa36f42..92362fb 100644 --- a/contracts/BaseEscrowFactory.sol +++ b/contracts/BaseEscrowFactory.sol @@ -80,6 +80,13 @@ abstract contract BaseEscrowFactory is IEscrowFactory, ResolverValidationExtensi LastValidated memory validated = lastValidated[key]; hashlock = validated.leaf; uint256 calculatedIndex = (order.makingAmount - remainingMakingAmount + makingAmount - 1) * partsAmount / order.makingAmount; + + // Check that this is not the first or the last fill. + if (order.makingAmount != remainingMakingAmount && remainingMakingAmount != makingAmount) { + uint256 calculatedIndexBefore = (order.makingAmount - remainingMakingAmount - 1) * partsAmount / order.makingAmount; + if (calculatedIndex <= calculatedIndexBefore) revert AlreadyUsedIndex(); + } + if ( (calculatedIndex + 1 != validated.index) && (calculatedIndex + 2 != validated.index || remainingMakingAmount != makingAmount) diff --git a/contracts/MerkleStorageInvalidator.sol b/contracts/MerkleStorageInvalidator.sol index ce27fb6..689f554 100644 --- a/contracts/MerkleStorageInvalidator.sol +++ b/contracts/MerkleStorageInvalidator.sol @@ -60,7 +60,6 @@ contract MerkleStorageInvalidator is IMerkleStorageInvalidator, ITakerInteractio } uint240 rootShortened = uint240(uint256(extraDataArgs.hashlockInfo)); bytes32 key = keccak256(abi.encodePacked(orderHash, rootShortened)); - if (takerData.idx < lastValidated[key].index) revert InvalidIndex(); bytes32 rootCalculated = takerData.proof.processProofCalldata(keccak256(abi.encodePacked(takerData.idx, takerData.secretHash))); if (uint240(uint256(rootCalculated)) != rootShortened) revert InvalidProof(); lastValidated[key] = LastValidated(takerData.idx + 1, takerData.secretHash); diff --git a/contracts/interfaces/IEscrowFactory.sol b/contracts/interfaces/IEscrowFactory.sol index 219c279..8d4ec5f 100644 --- a/contracts/interfaces/IEscrowFactory.sol +++ b/contracts/interfaces/IEscrowFactory.sol @@ -29,6 +29,7 @@ interface IEscrowFactory { uint256 chainId; } + error AlreadyUsedIndex(); error InsufficientEscrowBalance(); error InvalidCreationTime(); error InvalidSecretIndex(); diff --git a/contracts/interfaces/IMerkleStorageInvalidator.sol b/contracts/interfaces/IMerkleStorageInvalidator.sol index 72273d6..610b48f 100644 --- a/contracts/interfaces/IMerkleStorageInvalidator.sol +++ b/contracts/interfaces/IMerkleStorageInvalidator.sol @@ -19,7 +19,6 @@ interface IMerkleStorageInvalidator { } error AccessDenied(); - error InvalidIndex(); error InvalidProof(); /** diff --git a/test/integration/MerkleStorageInvalidator.t.sol b/test/integration/MerkleStorageInvalidator.t.sol index 32ad61d..b24ad4f 100644 --- a/test/integration/MerkleStorageInvalidator.t.sol +++ b/test/integration/MerkleStorageInvalidator.t.sol @@ -338,7 +338,7 @@ contract MerkleStorageInvalidatorIntTest is BaseSetup { assertEq(success, true); vm.prank(bob.addr); - vm.expectRevert(IMerkleStorageInvalidator.InvalidIndex.selector); + vm.expectRevert(IEscrowFactory.AlreadyUsedIndex.selector); limitOrderProtocol.fillOrderArgs( swapData.order, r, @@ -826,7 +826,7 @@ contract MerkleStorageInvalidatorIntTest is BaseSetup { ); assertEq(storedIndex, idx + 1); - // ------------ 2nd fill ------------ // + // ------------ 2nd fill, forged proof ------------ // uint256 makingAmount2 = makingAmount + 1; bytes32[] memory hashedSecretsLocal = new bytes32[](SECRETS_AMOUNT); bytes32[] memory hashedPairsLocal = new bytes32[](SECRETS_AMOUNT); @@ -871,6 +871,32 @@ contract MerkleStorageInvalidatorIntTest is BaseSetup { takerTraits2, args2 ); + + // ------------ 2nd fill, no taker interaction ------------ // + (TakerTraits takerTraits3, bytes memory args3) = CrossChainTestLib.buildTakerTraits( + true, // makingAmount + false, // unwrapWeth + false, // skipMakerPermit + false, // usePermit2 + srcClone2, // target + swapData.extension, + "", // interaction + 0 // threshold + ); + + (success,) = srcClone2.call{ value: SRC_SAFETY_DEPOSIT }(""); + assertEq(success, true); + + vm.prank(bob.addr); + vm.expectRevert(IEscrowFactory.AlreadyUsedIndex.selector); + limitOrderProtocol.fillOrderArgs( + swapData.order, + r, + vs, + makingAmount2, // amount + takerTraits3, + args3 + ); } /* solhint-enable func-name-mixedcase */ From e78a87c9980f3e2565390f13ce5961550ce0e103 Mon Sep 17 00:00:00 2001 From: byshape Date: Fri, 2 Aug 2024 11:38:08 +0100 Subject: [PATCH 2/2] Allow the order to be filled fully only with an extra secret --- .gas-snapshot | 47 +++---- contracts/BaseEscrowFactory.sol | 34 +++-- contracts/interfaces/IEscrowFactory.sol | 3 +- .../MerkleStorageInvalidator.t.sol | 130 ++++++++++++++++-- test/unit/EscrowFactory.t.sol | 2 +- 5 files changed, 169 insertions(+), 47 deletions(-) diff --git a/.gas-snapshot b/.gas-snapshot index b1d9e2c..1a028d3 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -1,5 +1,5 @@ -EscrowFactoryTest:test_MultipleFillsInvalidKey() (gas: 474261) -EscrowFactoryTest:test_MultipleFillsInvalidSecretsAmount() (gas: 473799) +EscrowFactoryTest:test_MultipleFillsInvalidKey() (gas: 474151) +EscrowFactoryTest:test_MultipleFillsInvalidSecretsAmount() (gas: 473793) EscrowFactoryTest:test_NoDeploymentForNotResolver() (gas: 190767) EscrowFactoryTest:test_NoInsufficientBalanceDeploymentForMaker() (gas: 128468) EscrowFactoryTest:test_NoInsufficientBalanceDeploymentForTaker() (gas: 27118) @@ -9,15 +9,15 @@ EscrowFactoryTest:test_NoUnsafeDeploymentForTaker() (gas: 34474) EscrowTest:test_CancelDst() (gas: 116028) EscrowTest:test_CancelDstDifferentTarget() (gas: 143286) EscrowTest:test_CancelDstWithNativeToken() (gas: 93622) -EscrowTest:test_CancelPublicSrc() (gas: 165457) +EscrowTest:test_CancelPublicSrc() (gas: 165442) EscrowTest:test_CancelResolverSrc() (gas: 168586) EscrowTest:test_CancelResolverSrcReceiver() (gas: 179340) -EscrowTest:test_NoAnyoneCancelDuringResolverCancelSrc() (gas: 163809) -EscrowTest:test_NoCallsWithInvalidImmutables() (gas: 286454) +EscrowTest:test_NoAnyoneCancelDuringResolverCancelSrc() (gas: 163829) +EscrowTest:test_NoCallsWithInvalidImmutables() (gas: 286452) EscrowTest:test_NoCancelByAnyoneDst() (gas: 121678) EscrowTest:test_NoCancelDuringWithdrawalDst() (gas: 121438) -EscrowTest:test_NoCancelDuringWithdrawalSrc() (gas: 163980) -EscrowTest:test_NoFailedNativeTokenTransferCancelSrc() (gas: 179317) +EscrowTest:test_NoCancelDuringWithdrawalSrc() (gas: 163938) +EscrowTest:test_NoFailedNativeTokenTransferCancelSrc() (gas: 179280) EscrowTest:test_NoFailedNativeTokenTransferWithdrawalDst() (gas: 154458) EscrowTest:test_NoFailedNativeTokenTransferWithdrawalDstNative() (gas: 83365) EscrowTest:test_NoFailedNativeTokenTransferWithdrawalSrc() (gas: 312313) @@ -28,16 +28,16 @@ EscrowTest:test_NoRescueFundsByAnyoneDst() (gas: 176275) EscrowTest:test_NoRescueFundsByAnyoneSrc() (gas: 209062) EscrowTest:test_NoRescueFundsEarlierDst() (gas: 175685) EscrowTest:test_NoRescueFundsEarlierSrc() (gas: 209019) -EscrowTest:test_NoWithdrawalByAnyoneSrc() (gas: 160840) +EscrowTest:test_NoWithdrawalByAnyoneSrc() (gas: 160820) EscrowTest:test_NoWithdrawalByNonResolverDst() (gas: 121384) EscrowTest:test_NoWithdrawalOutsideOfAllowedPeriodDst() (gas: 126271) -EscrowTest:test_NoWithdrawalOutsideOfAllowedPeriodSrc() (gas: 169623) +EscrowTest:test_NoWithdrawalOutsideOfAllowedPeriodSrc() (gas: 169632) EscrowTest:test_NoWithdrawalWithWrongSecretDst() (gas: 122749) EscrowTest:test_NoWithdrawalWithWrongSecretSrc() (gas: 164438) EscrowTest:test_PublicWithdrawSrc() (gas: 181729) EscrowTest:test_RescueFundsDst() (gas: 158179) EscrowTest:test_RescueFundsDstNative() (gas: 186642) -EscrowTest:test_RescueFundsSrc() (gas: 195445) +EscrowTest:test_RescueFundsSrc() (gas: 195453) EscrowTest:test_RescueFundsSrcNative() (gas: 197721) EscrowTest:test_WithdrawByAnyoneDst() (gas: 141240) EscrowTest:test_WithdrawByResolverDst() (gas: 142325) @@ -50,24 +50,25 @@ IntegrationEscrowFactoryTest:test_NoInsufficientBalanceDeploymentForMakerInt() ( IntegrationResolverMockTest:test_MockCancelDst() (gas: 157108) IntegrationResolverMockTest:test_MockCancelSrc() (gas: 353946) IntegrationResolverMockTest:test_MockDeployDst() (gas: 151449) -IntegrationResolverMockTest:test_MockDeploySrc() (gas: 365042) -IntegrationResolverMockTest:test_MockPublicCancelSrc() (gas: 392497) +IntegrationResolverMockTest:test_MockDeploySrc() (gas: 364909) +IntegrationResolverMockTest:test_MockPublicCancelSrc() (gas: 392454) IntegrationResolverMockTest:test_MockPublicWithdrawDst() (gas: 164765) IntegrationResolverMockTest:test_MockRescueFundsDst() (gas: 161014) IntegrationResolverMockTest:test_MockRescueFundsSrc() (gas: 382547) IntegrationResolverMockTest:test_MockWithdrawDst() (gas: 182848) IntegrationResolverMockTest:test_MockWithdrawToSrc() (gas: 354840) -MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllExtra() (gas: 923753) -MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllTwoFills() (gas: 922423) -MerkleStorageInvalidatorIntTest:test_MultipleFillsFillFirst() (gas: 707679) -MerkleStorageInvalidatorIntTest:test_MultipleFillsFillFirstTwoFills() (gas: 933077) -MerkleStorageInvalidatorIntTest:test_MultipleFillsFillLast() (gas: 707393) -MerkleStorageInvalidatorIntTest:test_MultipleFillsNoDeploymentWithoutValidation() (gas: 301520) -MerkleStorageInvalidatorIntTest:test_MultipleFillsNoReuseOfSecrets() (gas: 1052445) -MerkleStorageInvalidatorIntTest:test_MultipleFillsNoSecondDeploymentWithTheSameIndex() (gas: 786004) -MerkleStorageInvalidatorIntTest:test_MultipleFillsOddDivision() (gas: 443474) -MerkleStorageInvalidatorIntTest:test_MultipleFillsOneFill() (gas: 707716) -MerkleStorageInvalidatorIntTest:test_MultipleFillsTwoFills() (gas: 921543) +MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllExtra() (gas: 923650) +MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllFromLast() (gas: 922432) +MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllTwoFills() (gas: 922194) +MerkleStorageInvalidatorIntTest:test_MultipleFillsFillFirst() (gas: 707686) +MerkleStorageInvalidatorIntTest:test_MultipleFillsFillFirstTwoFills() (gas: 933080) +MerkleStorageInvalidatorIntTest:test_MultipleFillsFillLast() (gas: 707236) +MerkleStorageInvalidatorIntTest:test_MultipleFillsNoDeploymentWithoutValidation() (gas: 301343) +MerkleStorageInvalidatorIntTest:test_MultipleFillsNoReuseOfSecrets() (gas: 1052541) +MerkleStorageInvalidatorIntTest:test_MultipleFillsNoSecondDeploymentWithTheSameIndex() (gas: 786044) +MerkleStorageInvalidatorIntTest:test_MultipleFillsOddDivision() (gas: 443511) +MerkleStorageInvalidatorIntTest:test_MultipleFillsOneFill() (gas: 707723) +MerkleStorageInvalidatorIntTest:test_MultipleFillsTwoFills() (gas: 921559) TimelocksLibTest:test_NoTimelocksOverflow() (gas: 133743) TimelocksLibTest:test_getStartTimestamps() (gas: 16180) TimelocksLibTest:test_setDeployedAt() (gas: 5731) \ No newline at end of file diff --git a/contracts/BaseEscrowFactory.sol b/contracts/BaseEscrowFactory.sol index 92362fb..d312144 100644 --- a/contracts/BaseEscrowFactory.sol +++ b/contracts/BaseEscrowFactory.sol @@ -79,18 +79,9 @@ abstract contract BaseEscrowFactory is IEscrowFactory, ResolverValidationExtensi bytes32 key = keccak256(abi.encodePacked(orderHash, uint240(uint256(extraDataArgs.hashlockInfo)))); LastValidated memory validated = lastValidated[key]; hashlock = validated.leaf; - uint256 calculatedIndex = (order.makingAmount - remainingMakingAmount + makingAmount - 1) * partsAmount / order.makingAmount; - - // Check that this is not the first or the last fill. - if (order.makingAmount != remainingMakingAmount && remainingMakingAmount != makingAmount) { - uint256 calculatedIndexBefore = (order.makingAmount - remainingMakingAmount - 1) * partsAmount / order.makingAmount; - if (calculatedIndex <= calculatedIndexBefore) revert AlreadyUsedIndex(); + if (!_isValidPartialFill(makingAmount, remainingMakingAmount, order.makingAmount, partsAmount, validated.index)) { + revert InvalidPartialFill(); } - - if ( - (calculatedIndex + 1 != validated.index) && - (calculatedIndex + 2 != validated.index || remainingMakingAmount != makingAmount) - ) revert InvalidSecretIndex(); } else { hashlock = extraDataArgs.hashlockInfo; } @@ -172,4 +163,25 @@ abstract contract BaseEscrowFactory is IEscrowFactory, ResolverValidationExtensi function _deployEscrow(bytes32 salt, uint256 value, address implementation) internal virtual returns (address escrow) { escrow = implementation.cloneDeterministic(salt, value); } + + function _isValidPartialFill( + uint256 makingAmount, + uint256 remainingMakingAmount, + uint256 orderMakingAmount, + uint256 partsAmount, + uint256 validatedIndex + ) internal pure returns (bool) { + uint256 calculatedIndex = (orderMakingAmount - remainingMakingAmount + makingAmount - 1) * partsAmount / orderMakingAmount; + + if (remainingMakingAmount == makingAmount) { + // The last secret must be used for the last fill. + return (calculatedIndex + 2 == validatedIndex); + } else if (orderMakingAmount != remainingMakingAmount) { + // Calculate the previous fill index only if this is not the first fill. + uint256 prevCalculatedIndex = (orderMakingAmount - remainingMakingAmount - 1) * partsAmount / orderMakingAmount; + if (calculatedIndex == prevCalculatedIndex) return false; + } + + return calculatedIndex + 1 == validatedIndex; + } } diff --git a/contracts/interfaces/IEscrowFactory.sol b/contracts/interfaces/IEscrowFactory.sol index 8d4ec5f..1e40f61 100644 --- a/contracts/interfaces/IEscrowFactory.sol +++ b/contracts/interfaces/IEscrowFactory.sol @@ -29,10 +29,9 @@ interface IEscrowFactory { uint256 chainId; } - error AlreadyUsedIndex(); error InsufficientEscrowBalance(); error InvalidCreationTime(); - error InvalidSecretIndex(); + error InvalidPartialFill(); error InvalidSecretsAmount(); /** diff --git a/test/integration/MerkleStorageInvalidator.t.sol b/test/integration/MerkleStorageInvalidator.t.sol index b24ad4f..460edf2 100644 --- a/test/integration/MerkleStorageInvalidator.t.sol +++ b/test/integration/MerkleStorageInvalidator.t.sol @@ -81,14 +81,34 @@ contract MerkleStorageInvalidatorIntTest is BaseSetup { assertEq(usdc.balanceOf(srcClone), makingAmount); } + function _isValidPartialFill( + uint256 makingAmount, + uint256 remainingMakingAmount, + uint256 orderMakingAmount, + uint256 partsAmount, + uint256 validatedIndex + ) internal pure returns (bool) { + uint256 calculatedIndex = (orderMakingAmount - remainingMakingAmount + makingAmount - 1) * partsAmount / orderMakingAmount; + + if (remainingMakingAmount == makingAmount) { + // The last secret must be used for the last fill. + return (calculatedIndex + 2 == validatedIndex); + } else if (orderMakingAmount != remainingMakingAmount) { + // Calculate the previous fill index only if this is not the first fill. + uint256 prevCalculatedIndex = (orderMakingAmount - remainingMakingAmount - 1) * partsAmount / orderMakingAmount; + if (calculatedIndex == prevCalculatedIndex) return false; + } + + return calculatedIndex + 1 == validatedIndex; + } + function testFuzz_MultipleFillsOneFillPassAndFail(uint256 makingAmount, uint256 partsAmount, uint256 idx) public { makingAmount = bound(makingAmount, 1, MAKING_AMOUNT); partsAmount = bound(partsAmount, 2, 100); idx = bound(idx, 0, partsAmount); uint256 secretsAmount = partsAmount + 1; - uint256 idxCalculated = partsAmount * (makingAmount - 1) / MAKING_AMOUNT; - bool shouldFail = (idxCalculated != idx) && ((idx != partsAmount) || (makingAmount != MAKING_AMOUNT)); + bool shouldFail = !_isValidPartialFill(makingAmount, MAKING_AMOUNT, MAKING_AMOUNT, partsAmount, idx + 1); bytes32[] memory hashedSecretsLocal = new bytes32[](secretsAmount); bytes32[] memory hashedPairsLocal = new bytes32[](secretsAmount); @@ -131,7 +151,7 @@ contract MerkleStorageInvalidatorIntTest is BaseSetup { vm.prank(bob.addr); if (shouldFail) { - vm.expectRevert(IEscrowFactory.InvalidSecretIndex.selector); + vm.expectRevert(IEscrowFactory.InvalidPartialFill.selector); } limitOrderProtocol.fillOrderArgs( swapData.order, @@ -258,7 +278,7 @@ contract MerkleStorageInvalidatorIntTest is BaseSetup { assertEq(success, true); vm.prank(bob.addr); - vm.expectRevert(IEscrowFactory.InvalidSecretIndex.selector); + vm.expectRevert(IEscrowFactory.InvalidPartialFill.selector); limitOrderProtocol.fillOrderArgs( swapData.order, r, @@ -338,7 +358,7 @@ contract MerkleStorageInvalidatorIntTest is BaseSetup { assertEq(success, true); vm.prank(bob.addr); - vm.expectRevert(IEscrowFactory.AlreadyUsedIndex.selector); + vm.expectRevert(IEscrowFactory.InvalidPartialFill.selector); limitOrderProtocol.fillOrderArgs( swapData.order, r, @@ -489,7 +509,7 @@ contract MerkleStorageInvalidatorIntTest is BaseSetup { } function test_MultipleFillsFillLast() public { - uint256 idx = PARTS_AMOUNT - 1; + uint256 idx = PARTS_AMOUNT; // Use the "extra" secret bytes32[] memory proof = merkle.getProof(hashedPairs, idx); assert(merkle.verifyProof(root, proof, hashedPairs[idx])); @@ -533,9 +553,99 @@ contract MerkleStorageInvalidatorIntTest is BaseSetup { assertEq(usdc.balanceOf(srcClone), MAKING_AMOUNT); } + function test_MultipleFillsFillAllFromLast() public { + uint256 idx = PARTS_AMOUNT - 1; + uint256 makingAmount = MAKING_AMOUNT - 1; + bytes32[] memory proof = merkle.getProof(hashedPairs, idx); + assert(merkle.verifyProof(root, proof, hashedPairs[idx])); + + CrossChainTestLib.SwapData memory swapData = _prepareDataSrcHashlock(rootPlusAmount, false, true); + + swapData.immutables.hashlock = hashedSecrets[idx]; + swapData.immutables.amount = makingAmount; + address srcClone = escrowFactory.addressOfEscrowSrc(swapData.immutables); + + (uint8 v, bytes32 r, bytes32 s) = vm.sign(alice.privateKey, swapData.orderHash); + bytes32 vs = bytes32((uint256(v - 27) << 255)) | s; + + bytes memory interaction = abi.encodePacked(escrowFactory, abi.encode(proof, idx, hashedSecrets[idx])); + + (TakerTraits takerTraits, bytes memory args) = CrossChainTestLib.buildTakerTraits( + true, // makingAmount + false, // unwrapWeth + false, // skipMakerPermit + false, // usePermit2 + srcClone, // target + swapData.extension, + interaction, + 0 // threshold + ); + + (bool success,) = srcClone.call{ value: SRC_SAFETY_DEPOSIT }(""); + assertEq(success, true); + + + vm.prank(bob.addr); + limitOrderProtocol.fillOrderArgs( + swapData.order, + r, + vs, + makingAmount, // amount + takerTraits, + args + ); + + assertEq(usdc.balanceOf(srcClone), makingAmount); + + // ------------ 2nd fill ------------ // + idx = PARTS_AMOUNT; // Use the "extra" secret + uint256 makingAmount2 = MAKING_AMOUNT - makingAmount; + proof = merkle.getProof(hashedPairs, idx); + assert(merkle.verifyProof(root, proof, hashedPairs[idx])); + + swapData.immutables.hashlock = hashedSecrets[idx]; + swapData.immutables.amount = makingAmount2; + address srcClone2 = escrowFactory.addressOfEscrowSrc(swapData.immutables); + + (v, r, s) = vm.sign(alice.privateKey, swapData.orderHash); + vs = bytes32((uint256(v - 27) << 255)) | s; + + interaction = abi.encodePacked(escrowFactory, abi.encode(proof, idx, hashedSecrets[idx])); + + (TakerTraits takerTraits2, bytes memory args2) = CrossChainTestLib.buildTakerTraits( + true, // makingAmount + false, // unwrapWeth + false, // skipMakerPermit + false, // usePermit2 + srcClone2, // target + swapData.extension, + interaction, + 0 // threshold + ); + + (success,) = srcClone2.call{ value: SRC_SAFETY_DEPOSIT }(""); + assertEq(success, true); + + vm.prank(bob.addr); + limitOrderProtocol.fillOrderArgs( + swapData.order, + r, + vs, + makingAmount2, // amount + takerTraits2, + args2 + ); + + assertEq(usdc.balanceOf(address(srcClone2)), makingAmount2); + (uint256 storedIndex,) = IMerkleStorageInvalidator(escrowFactory).lastValidated( + keccak256(abi.encodePacked(swapData.orderHash, uint240(uint256(root)))) + ); + assertEq(storedIndex, PARTS_AMOUNT + 1); + } + function test_MultipleFillsFillAllTwoFills() public { uint256 idx = PARTS_AMOUNT - 2; - uint256 makingAmount = MAKING_AMOUNT * (idx + 1) / PARTS_AMOUNT; + uint256 makingAmount = MAKING_AMOUNT * (idx + 1) / PARTS_AMOUNT - 1; bytes32[] memory proof = merkle.getProof(hashedPairs, idx); assert(merkle.verifyProof(root, proof, hashedPairs[idx])); @@ -578,7 +688,7 @@ contract MerkleStorageInvalidatorIntTest is BaseSetup { assertEq(usdc.balanceOf(srcClone), makingAmount); // ------------ 2nd fill ------------ // - idx = PARTS_AMOUNT - 1; + idx = PARTS_AMOUNT; // Use the "extra" secret uint256 makingAmount2 = MAKING_AMOUNT - makingAmount; proof = merkle.getProof(hashedPairs, idx); assert(merkle.verifyProof(root, proof, hashedPairs[idx])); @@ -620,7 +730,7 @@ contract MerkleStorageInvalidatorIntTest is BaseSetup { (uint256 storedIndex,) = IMerkleStorageInvalidator(escrowFactory).lastValidated( keccak256(abi.encodePacked(swapData.orderHash, uint240(uint256(root)))) ); - assertEq(storedIndex, PARTS_AMOUNT); + assertEq(storedIndex, PARTS_AMOUNT + 1); } function test_MultipleFillsFillAllExtra() public { @@ -888,7 +998,7 @@ contract MerkleStorageInvalidatorIntTest is BaseSetup { assertEq(success, true); vm.prank(bob.addr); - vm.expectRevert(IEscrowFactory.AlreadyUsedIndex.selector); + vm.expectRevert(IEscrowFactory.InvalidPartialFill.selector); limitOrderProtocol.fillOrderArgs( swapData.order, r, diff --git a/test/unit/EscrowFactory.t.sol b/test/unit/EscrowFactory.t.sol index 29e7af8..7eadd84 100644 --- a/test/unit/EscrowFactory.t.sol +++ b/test/unit/EscrowFactory.t.sol @@ -270,7 +270,7 @@ contract EscrowFactoryTest is BaseSetup { CrossChainTestLib.SwapData memory swapData = _prepareDataSrcHashlock(rootPlusAmount, false, true); vm.prank(address(limitOrderProtocol)); - vm.expectRevert(IEscrowFactory.InvalidSecretIndex.selector); + vm.expectRevert(IEscrowFactory.InvalidPartialFill.selector); escrowFactory.postInteraction( swapData.order, "", // extension