Skip to content

Commit

Permalink
Merge pull request #96 from 1inch/audits/SC-1218-Forbid-to-fill-the-s…
Browse files Browse the repository at this point in the history 
…ame-part-multiple

Forbid to fill the same part of an order more than once
  • Loading branch information
@byshape
byshape authored Aug 2, 2024
2 parents 19f2bf4 + e78a87c commit 10c0b71
Show file tree
Hide file tree
Showing 7 changed files with 197 additions and 43 deletions.
49 changes: 25 additions & 24 deletions .gas-snapshot
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
EscrowFactoryTest:test_MultipleFillsInvalidKey() (gas: 474203)
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)
Expand All @@ -9,65 +9,66 @@ 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: 163829)
EscrowTest:test_NoCallsWithInvalidImmutables() (gas: 286454)
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: 312353)
EscrowTest:test_NoFailedNativeTokenTransferWithdrawalSrc() (gas: 312313)
EscrowTest:test_NoPublicCancelDuringPrivateCancellationSrc() (gas: 163124)
EscrowTest:test_NoPublicWithdrawOutsideOfAllowedPeriodDst() (gas: 126855)
EscrowTest:test_NoPublicWithdrawalOutsideOfAllowedPeriodSrc() (gas: 169069)
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: 195453)
EscrowTest:test_RescueFundsSrcNative() (gas: 197720)
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)
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: 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: 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)
29 changes: 24 additions & 5 deletions contracts/BaseEscrowFactory.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,11 +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;
if (
(calculatedIndex + 1 != validated.index) &&
(calculatedIndex + 2 != validated.index || remainingMakingAmount != makingAmount)
) revert InvalidSecretIndex();
if (!_isValidPartialFill(makingAmount, remainingMakingAmount, order.makingAmount, partsAmount, validated.index)) {
revert InvalidPartialFill();
}
} else {
hashlock = extraDataArgs.hashlockInfo;
}
Expand Down Expand Up @@ -165,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;
}
}
1 change: 0 additions & 1 deletion contracts/MerkleStorageInvalidator.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion contracts/interfaces/IEscrowFactory.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ interface IEscrowFactory {

error InsufficientEscrowBalance();
error InvalidCreationTime();
error InvalidSecretIndex();
error InvalidPartialFill();
error InvalidSecretsAmount();

/**
Expand Down
1 change: 0 additions & 1 deletion contracts/interfaces/IMerkleStorageInvalidator.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ interface IMerkleStorageInvalidator {
}

error AccessDenied();
error InvalidIndex();
error InvalidProof();

/**
Expand Down
156 changes: 146 additions & 10 deletions test/integration/MerkleStorageInvalidator.t.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -338,7 +358,7 @@ contract MerkleStorageInvalidatorIntTest is BaseSetup {
assertEq(success, true);

vm.prank(bob.addr);
vm.expectRevert(IMerkleStorageInvalidator.InvalidIndex.selector);
vm.expectRevert(IEscrowFactory.InvalidPartialFill.selector);
limitOrderProtocol.fillOrderArgs(
swapData.order,
r,
Expand Down Expand Up @@ -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]));

Expand Down Expand Up @@ -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]));

Expand Down Expand Up @@ -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]));
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -826,7 +936,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);
Expand Down Expand Up @@ -871,6 +981,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.InvalidPartialFill.selector);
limitOrderProtocol.fillOrderArgs(
swapData.order,
r,
vs,
makingAmount2, // amount
takerTraits3,
args3
);
}

/* solhint-enable func-name-mixedcase */
Expand Down
Loading

0 comments on commit 10c0b71

Please sign in to comment.