diff --git a/.gas-snapshot b/.gas-snapshot index ccd2a75..ae0be3b 100644 --- a/.gas-snapshot +++ b/.gas-snapshot @@ -1,6 +1,6 @@ EscrowFactoryTest:test_MultipleFillsInvalidKey() (gas: 474076) EscrowFactoryTest:test_MultipleFillsInvalidSecretsAmount() (gas: 473718) -EscrowFactoryTest:test_NoDeploymentForNotResolver() (gas: 190767) +EscrowFactoryTest:test_NoDeploymentForNotResolver() (gas: 190780) EscrowFactoryTest:test_NoInsufficientBalanceDeploymentForMaker() (gas: 128460) EscrowFactoryTest:test_NoInsufficientBalanceDeploymentForTaker() (gas: 27118) EscrowFactoryTest:test_NoInsufficientBalanceNativeDeploymentForMaker() (gas: 121696) @@ -10,16 +10,16 @@ EscrowTest:test_CancelDst() (gas: 116126) EscrowTest:test_CancelDstDifferentTarget() (gas: 143384) EscrowTest:test_CancelDstWithNativeToken() (gas: 93710) EscrowTest:test_CancelPublicSrc() (gas: 165464) -EscrowTest:test_CancelResolverSrc() (gas: 168624) +EscrowTest:test_CancelResolverSrc() (gas: 168607) EscrowTest:test_CancelResolverSrcReceiver() (gas: 179361) EscrowTest:test_NoAnyoneCancelDuringResolverCancelSrc() (gas: 163856) EscrowTest:test_NoCallsWithInvalidImmutables() (gas: 286518) EscrowTest:test_NoCancelByAnyoneDst() (gas: 121726) EscrowTest:test_NoCancelDuringWithdrawalDst() (gas: 121486) -EscrowTest:test_NoCancelDuringWithdrawalSrc() (gas: 163959) +EscrowTest:test_NoCancelDuringWithdrawalSrc() (gas: 164001) EscrowTest:test_NoFailedNativeTokenTransferCancelSrc() (gas: 179339) EscrowTest:test_NoFailedNativeTokenTransferWithdrawalDst() (gas: 154544) -EscrowTest:test_NoFailedNativeTokenTransferWithdrawalSrc() (gas: 312369) +EscrowTest:test_NoFailedNativeTokenTransferWithdrawalSrc() (gas: 312329) EscrowTest:test_NoPublicCancelDuringPrivateCancellationSrc() (gas: 163166) EscrowTest:test_NoPublicWithdrawOutsideOfAllowedPeriodDst() (gas: 126910) EscrowTest:test_NoPublicWithdrawalOutsideOfAllowedPeriodSrc() (gas: 169085) @@ -31,10 +31,10 @@ EscrowTest:test_NoRevertFailedNativeTokenTransferWithdrawalDstNative() (gas: 911 EscrowTest:test_NoWithdrawalByAnyoneSrc() (gas: 160873) EscrowTest:test_NoWithdrawalByNonResolverDst() (gas: 121439) EscrowTest:test_NoWithdrawalOutsideOfAllowedPeriodDst() (gas: 126326) -EscrowTest:test_NoWithdrawalOutsideOfAllowedPeriodSrc() (gas: 169627) +EscrowTest:test_NoWithdrawalOutsideOfAllowedPeriodSrc() (gas: 169636) EscrowTest:test_NoWithdrawalWithWrongSecretDst() (gas: 122776) EscrowTest:test_NoWithdrawalWithWrongSecretSrc() (gas: 164478) -EscrowTest:test_PublicWithdrawSrc() (gas: 181761) +EscrowTest:test_PublicWithdrawSrc() (gas: 181745) EscrowTest:test_RescueFundsDst() (gas: 158304) EscrowTest:test_RescueFundsDstNative() (gas: 186735) EscrowTest:test_RescueFundsSrc() (gas: 195445) @@ -45,13 +45,14 @@ EscrowTest:test_WithdrawByResolverDstNative() (gas: 97831) EscrowTest:test_WithdrawByResolverPublicDst() (gas: 141757) EscrowTest:test_WithdrawSrc() (gas: 186505) EscrowTest:test_WithdrawSrcTo() (gas: 191335) -IntegrationEscrowFactoryTest:test_DeployCloneForMakerNonWhitelistedResolverInt() (gas: 473480) -IntegrationEscrowFactoryTest:test_NoInsufficientBalanceDeploymentForMakerInt() (gas: 341110) +IntegrationEscrowFactoryTest:test_DeployCloneForMakerNonWhitelistedResolverInt() (gas: 473542) +IntegrationEscrowFactoryTest:test_NoInsufficientBalanceDeploymentForMakerInt() (gas: 341264) +IntegrationEscrowFactoryTest:test_NoResolverReentrancy() (gas: 2065187) IntegrationResolverMockTest:test_MockCancelDst() (gas: 157184) -IntegrationResolverMockTest:test_MockCancelSrc() (gas: 353972) +IntegrationResolverMockTest:test_MockCancelSrc() (gas: 353957) IntegrationResolverMockTest:test_MockDeployDst() (gas: 151470) IntegrationResolverMockTest:test_MockDeploySrc() (gas: 364921) -IntegrationResolverMockTest:test_MockPublicCancelSrc() (gas: 392502) +IntegrationResolverMockTest:test_MockPublicCancelSrc() (gas: 392459) IntegrationResolverMockTest:test_MockPublicWithdrawDst() (gas: 164841) IntegrationResolverMockTest:test_MockRescueFundsDst() (gas: 161103) IntegrationResolverMockTest:test_MockRescueFundsSrc() (gas: 382583) @@ -60,16 +61,16 @@ IntegrationResolverMockTest:test_MockWithdrawToSrc() (gas: 354839) MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllExtra() (gas: 923576) MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllFromLast() (gas: 922329) MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllTwoFills() (gas: 922136) -MerkleStorageInvalidatorIntTest:test_MultipleFillsFillFirst() (gas: 707614) +MerkleStorageInvalidatorIntTest:test_MultipleFillsFillFirst() (gas: 707638) MerkleStorageInvalidatorIntTest:test_MultipleFillsFillFirstTwoFills() (gas: 932969) -MerkleStorageInvalidatorIntTest:test_MultipleFillsFillLast() (gas: 707218) +MerkleStorageInvalidatorIntTest:test_MultipleFillsFillLast() (gas: 707297) MerkleStorageInvalidatorIntTest:test_MultipleFillsNoDeploymentWithoutValidation() (gas: 301343) -MerkleStorageInvalidatorIntTest:test_MultipleFillsNoReuseOfSecrets() (gas: 1068017) +MerkleStorageInvalidatorIntTest:test_MultipleFillsNoReuseOfSecrets() (gas: 1069228) MerkleStorageInvalidatorIntTest:test_MultipleFillsNoSecondDeploymentWithTheSameIndex() (gas: 786011) MerkleStorageInvalidatorIntTest:test_MultipleFillsOddDivision() (gas: 444783) -MerkleStorageInvalidatorIntTest:test_MultipleFillsOneFill() (gas: 707710) -MerkleStorageInvalidatorIntTest:test_MultipleFillsTwoFills() (gas: 921433) -MerkleStorageInvalidatorTest:test_ShortenedProofIsNotValid() (gas: 230552) +MerkleStorageInvalidatorIntTest:test_MultipleFillsOneFill() (gas: 707674) +MerkleStorageInvalidatorIntTest:test_MultipleFillsTwoFills() (gas: 921440) +MerkleStorageInvalidatorTest:test_ShortenedProofIsNotValid() (gas: 230605) TimelocksLibTest:test_NoTimelocksOverflow() (gas: 156200) TimelocksLibTest:test_getStartTimestamps() (gas: 16207) TimelocksLibTest:test_setDeployedAt() (gas: 5741) \ No newline at end of file diff --git a/test/integration/EscrowFactory.t.sol b/test/integration/EscrowFactory.t.sol index fd0f03f..20d7bdd 100644 --- a/test/integration/EscrowFactory.t.sol +++ b/test/integration/EscrowFactory.t.sol @@ -2,12 +2,14 @@ pragma solidity 0.8.23; import { TakerTraits } from "limit-order-protocol/contracts/libraries/TakerTraitsLib.sol"; +import { Merkle } from "murky/src/Merkle.sol"; import { Address } from "solidity-utils/contracts/libraries/AddressLib.sol"; import { IEscrowFactory } from "contracts/interfaces/IEscrowFactory.sol"; import { BaseSetup } from "../utils/BaseSetup.sol"; import { CrossChainTestLib } from "../utils/libraries/CrossChainTestLib.sol"; +import { ResolverReentrancy } from "../utils/mocks/ResolverReentrancy.sol"; contract IntegrationEscrowFactoryTest is BaseSetup { function setUp() public virtual override { @@ -148,5 +150,65 @@ contract IntegrationEscrowFactoryTest is BaseSetup { ); } + function test_NoResolverReentrancy() public { + ResolverReentrancy badResolver = new ResolverReentrancy(escrowFactory, limitOrderProtocol, address(this)); + resolvers[0] = address(badResolver); + vm.deal(address(badResolver), 100 ether); + + uint256 partsAmount = 100; + uint256 secretsAmount = partsAmount + 1; + bytes32[] memory hashedSecrets = new bytes32[](secretsAmount); + bytes32[] memory hashedPairs = new bytes32[](secretsAmount); + for (uint64 i = 0; i < secretsAmount; i++) { + // Note: This is not production-ready code. Use cryptographically secure random to generate secrets. + hashedSecrets[i] = keccak256(abi.encodePacked(i)); + hashedPairs[i] = keccak256(abi.encodePacked(i, hashedSecrets[i])); + } + Merkle merkle = new Merkle(); + bytes32 root = merkle.getRoot(hashedPairs); + bytes32 rootPlusAmount = bytes32(partsAmount << 240 | uint240(uint256(root))); + uint256 idx = 0; + uint256 makingAmount = MAKING_AMOUNT / partsAmount; + bytes32[] memory proof = merkle.getProof(hashedPairs, idx); + assert(merkle.verifyProof(root, proof, hashedPairs[idx])); + + vm.warp(1710288000); // set current timestamp + (timelocks, timelocksDst) = CrossChainTestLib.setTimelocks(srcTimelocks, dstTimelocks); + + + CrossChainTestLib.SwapData memory swapData = _prepareDataSrcHashlock(rootPlusAmount, false, true); + + swapData.immutables.hashlock = hashedSecrets[idx]; + swapData.immutables.amount = makingAmount - 2; + + (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(address(badResolver)); + bytes memory interactionFull = abi.encodePacked(interaction, escrowFactory, abi.encode(proof, idx, hashedSecrets[idx])); + + (TakerTraits takerTraits, bytes memory args) = CrossChainTestLib.buildTakerTraits( + true, // makingAmount + false, // unwrapWeth + false, // skipMakerPermit + false, // usePermit2 + address(0), // target + swapData.extension, // extension + interactionFull, + 0 // threshold + ); + + vm.expectRevert(IEscrowFactory.InvalidPartialFill.selector); + badResolver.deploySrc( + swapData.immutables, + swapData.order, + r, + vs, + makingAmount - 2, + takerTraits, + args + ); + } + /* solhint-enable func-name-mixedcase */ } diff --git a/test/utils/mocks/ResolverReentrancy.sol b/test/utils/mocks/ResolverReentrancy.sol new file mode 100644 index 0000000..2b2626c --- /dev/null +++ b/test/utils/mocks/ResolverReentrancy.sol @@ -0,0 +1,91 @@ +// SPDX-License-Identifier: MIT + +pragma solidity 0.8.23; + +import { Ownable } from "openzeppelin-contracts/contracts/access/Ownable.sol"; + +import { IOrderMixin } from "limit-order-protocol/contracts/interfaces/IOrderMixin.sol"; +import { TakerTraits } from "limit-order-protocol/contracts/libraries/TakerTraitsLib.sol"; + +import { IBaseEscrow } from "../../../contracts/interfaces/IBaseEscrow.sol"; +import { IEscrowFactory } from "../../../contracts/interfaces/IEscrowFactory.sol"; +import { TimelocksLib } from "../../../contracts/libraries/TimelocksLib.sol"; + +contract ResolverReentrancy is Ownable { + uint256 private constant _ARGS_INTERACTION_LENGTH_OFFSET = 200; + IEscrowFactory private immutable _FACTORY; + IOrderMixin private immutable _LOP; + bytes32 private _r; + bytes32 private _vs; + TakerTraits private _takerTraits; + IBaseEscrow.Immutables private _immutables; + + error AccessDenied(); + + /// @notice Only limit order protocol can call this contract. + modifier onlyLOP() { + if (msg.sender != address(_LOP)) { + revert AccessDenied(); + } + _; + } + + constructor(IEscrowFactory factory, IOrderMixin lop, address initialOwner) Ownable(initialOwner) { + _FACTORY = factory; + _LOP = lop; + } + + receive() external payable {} // solhint-disable-line no-empty-blocks + + /** + * @notice See {IResolverExample-deploySrc}. + */ + function deploySrc( + IBaseEscrow.Immutables calldata immutables, + IOrderMixin.Order calldata order, + bytes32 r, + bytes32 vs, + uint256 amount, + TakerTraits takerTraits, + bytes calldata args + ) external onlyOwner { + IBaseEscrow.Immutables memory immutablesMem = immutables; + immutablesMem.timelocks = TimelocksLib.setDeployedAt(immutables.timelocks, block.timestamp); + address computed = _FACTORY.addressOfEscrowSrc(immutablesMem); + (bool success,) = address(computed).call{ value: immutablesMem.safetyDeposit }(""); + if (!success) revert IBaseEscrow.NativeTokenSendingFailure(); + + // _ARGS_HAS_TARGET = 1 << 251 + takerTraits = TakerTraits.wrap(TakerTraits.unwrap(takerTraits) | uint256(1 << 251)); + bytes memory argsMem = abi.encodePacked(computed, args); + _immutables = immutables; + _r = r; + _vs = vs; + _takerTraits = takerTraits; + _LOP.fillOrderArgs(order, r, vs, amount, takerTraits, argsMem); + } + + function takerInteraction( + IOrderMixin.Order calldata order, + bytes calldata extension, + bytes32 /* orderHash */, + address /* taker */, + uint256 /* makingAmount */, + uint256 /* takingAmount */, + uint256 /* remainingMakingAmount */, + bytes calldata extraData + ) external onlyLOP { + _immutables.amount = 1; + address computed = _FACTORY.addressOfEscrowSrc(_immutables); + (bool success,) = address(computed).call{ value: _immutables.safetyDeposit }(""); + if (!success) revert IBaseEscrow.NativeTokenSendingFailure(); + + _takerTraits = TakerTraits.wrap( + TakerTraits.unwrap(_takerTraits) & + ~(uint256(type(uint24).max) << _ARGS_INTERACTION_LENGTH_OFFSET) | + (extraData.length << _ARGS_INTERACTION_LENGTH_OFFSET) + ); + bytes memory argsMem = abi.encodePacked(computed, extension, extraData); + _LOP.fillOrderArgs(order, _r, _vs, 1, _takerTraits, argsMem); + } +}