Merge pull request #138 from 1inch/fix/additional-amount-checks

[PT1-97] Fixes after OpenZeppelin audit review

Author: galekseev

.gas-snapshot

@@ -1,23 +1,23 @@
 EscrowCancelTest:test_CancelDst() (gas: 123967)
 EscrowCancelTest:test_CancelDstDifferentTarget() (gas: 151802)
 EscrowCancelTest:test_CancelDstWithNativeToken() (gas: 101063)
-EscrowCancelTest:test_CancelPublicSrc() (gas: 182872)
-EscrowCancelTest:test_CancelResolverSrc() (gas: 180725)
-EscrowCancelTest:test_CancelResolverSrcReceiver() (gas: 202247)
-EscrowCancelTest:test_NoAnyoneCancelDuringResolverCancelSrc() (gas: 174648)
-EscrowCancelTest:test_NoCallsWithInvalidImmutables() (gas: 303324)
+EscrowCancelTest:test_CancelPublicSrc() (gas: 182952)
+EscrowCancelTest:test_CancelResolverSrc() (gas: 180805)
+EscrowCancelTest:test_CancelResolverSrcReceiver() (gas: 202327)
+EscrowCancelTest:test_NoAnyoneCancelDuringResolverCancelSrc() (gas: 174728)
+EscrowCancelTest:test_NoCallsWithInvalidImmutables() (gas: 303404)
 EscrowCancelTest:test_NoCancelByAnyoneDst() (gas: 128060)
 EscrowCancelTest:test_NoCancelDuringWithdrawalDst() (gas: 128407)
-EscrowCancelTest:test_NoCancelDuringWithdrawalSrc() (gas: 174945)
-EscrowCancelTest:test_NoFailedNativeTokenTransferCancelSrc() (gas: 230224)
-EscrowCancelTest:test_NoPublicCallsByAnyone() (gas: 301744)
-EscrowCancelTest:test_NoPublicCancelDuringPrivateCancellationSrc() (gas: 179665)
-EscrowFactoryTest:test_MultipleFillsInvalidKey() (gas: 482346)
-EscrowFactoryTest:test_MultipleFillsInvalidSecretsAmount() (gas: 482090)
+EscrowCancelTest:test_NoCancelDuringWithdrawalSrc() (gas: 175025)
+EscrowCancelTest:test_NoFailedNativeTokenTransferCancelSrc() (gas: 230304)
+EscrowCancelTest:test_NoPublicCallsByAnyone() (gas: 301824)
+EscrowCancelTest:test_NoPublicCancelDuringPrivateCancellationSrc() (gas: 179745)
+EscrowFactoryTest:test_MultipleFillsInvalidKey() (gas: 482426)
+EscrowFactoryTest:test_MultipleFillsInvalidSecretsAmount() (gas: 482170)
 EscrowFactoryTest:test_NoDeploymentForNotResolver() (gas: 156992)
-EscrowFactoryTest:test_NoInsufficientBalanceDeploymentForMaker() (gas: 139700)
+EscrowFactoryTest:test_NoInsufficientBalanceDeploymentForMaker() (gas: 139780)
 EscrowFactoryTest:test_NoInsufficientBalanceDeploymentForTaker() (gas: 32999)
-EscrowFactoryTest:test_NoInsufficientBalanceNativeDeploymentForMaker() (gas: 132574)
+EscrowFactoryTest:test_NoInsufficientBalanceNativeDeploymentForMaker() (gas: 132654)
 EscrowFactoryTest:test_NoInsufficientBalanceNativeDeploymentForTaker() (gas: 33287)
 EscrowFactoryTest:test_NoRescueFundsERC20NotOwner() (gas: 60457)
 EscrowFactoryTest:test_NoRescueFundsNativeNotOwner() (gas: 24875)
@@ -26,56 +26,56 @@ EscrowFactoryTest:test_RescueFundsERC20() (gas: 67574)
 EscrowFactoryTest:test_RescueFundsNative() (gas: 31067)
 EscrowTest:test_NoFailedNativeTokenTransferWithdrawalDst() (gas: 263622)
 EscrowTest:test_NoFailedNativeTokenTransferWithdrawalDstNative() (gas: 127238)
-EscrowTest:test_NoFailedNativeTokenTransferWithdrawalSrc() (gas: 361667)
+EscrowTest:test_NoFailedNativeTokenTransferWithdrawalSrc() (gas: 361747)
 EscrowTest:test_NoPublicWithdrawOutsideOfAllowedPeriodDst() (gas: 145135)
-EscrowTest:test_NoPublicWithdrawalOutsideOfAllowedPeriodSrc() (gas: 191532)
+EscrowTest:test_NoPublicWithdrawalOutsideOfAllowedPeriodSrc() (gas: 191612)
 EscrowTest:test_NoRescueFundsByAnyoneDst() (gas: 251799)
-EscrowTest:test_NoRescueFundsByAnyoneSrc() (gas: 223354)
+EscrowTest:test_NoRescueFundsByAnyoneSrc() (gas: 223434)
 EscrowTest:test_NoRescueFundsEarlierDst() (gas: 251565)
-EscrowTest:test_NoRescueFundsEarlierSrc() (gas: 223482)
-EscrowTest:test_NoWithdrawalByAnyoneSrc() (gas: 172308)
+EscrowTest:test_NoRescueFundsEarlierSrc() (gas: 223562)
+EscrowTest:test_NoWithdrawalByAnyoneSrc() (gas: 172388)
 EscrowTest:test_NoWithdrawalByNonResolverDst() (gas: 128322)
 EscrowTest:test_NoWithdrawalOutsideOfAllowedPeriodDst() (gas: 133787)
-EscrowTest:test_NoWithdrawalOutsideOfAllowedPeriodSrc() (gas: 181329)
+EscrowTest:test_NoWithdrawalOutsideOfAllowedPeriodSrc() (gas: 181409)
 EscrowTest:test_NoWithdrawalWithWrongSecretDst() (gas: 129738)
-EscrowTest:test_NoWithdrawalWithWrongSecretSrc() (gas: 176159)
-EscrowTest:test_PublicWithdrawSrc() (gas: 199676)
+EscrowTest:test_NoWithdrawalWithWrongSecretSrc() (gas: 176239)
+EscrowTest:test_PublicWithdrawSrc() (gas: 199756)
 EscrowTest:test_RescueFundsDst() (gas: 231195)
 EscrowTest:test_RescueFundsDstNative() (gas: 262234)
-EscrowTest:test_RescueFundsSrc() (gas: 209951)
-EscrowTest:test_RescueFundsSrcNative() (gas: 212590)
+EscrowTest:test_RescueFundsSrc() (gas: 210031)
+EscrowTest:test_RescueFundsSrcNative() (gas: 212670)
 EscrowTest:test_WithdrawByAnyoneDst() (gas: 220204)
 EscrowTest:test_WithdrawByResolverDst() (gas: 213095)
 EscrowTest:test_WithdrawByResolverDstNative() (gas: 136994)
 EscrowTest:test_WithdrawByResolverPublicDst() (gas: 215234)
-EscrowTest:test_WithdrawSrc() (gas: 198985)
-EscrowTest:test_WithdrawSrcTo() (gas: 203858)
+EscrowTest:test_WithdrawSrc() (gas: 199065)
+EscrowTest:test_WithdrawSrcTo() (gas: 203938)
 FeeCalcLibTest:test_getFeeAmounts() (gas: 29011)
-IntegrationEscrowFactoryTest:test_DeployCloneForMakerNonWhitelistedResolverInt() (gas: 433651)
-IntegrationEscrowFactoryTest:test_NoInsufficientBalanceDeploymentForMakerInt() (gas: 355292)
-IntegrationEscrowFactoryTest:test_NoResolverReentrancy() (gas: 2354724)
+IntegrationEscrowFactoryTest:test_DeployCloneForMakerNonWhitelistedResolverInt() (gas: 433731)
+IntegrationEscrowFactoryTest:test_NoInsufficientBalanceDeploymentForMakerInt() (gas: 355372)
+IntegrationEscrowFactoryTest:test_NoResolverReentrancy() (gas: 2354804)
 IntegrationResolverMockTest:test_MockCancelDst() (gas: 168508)
-IntegrationResolverMockTest:test_MockCancelSrc() (gas: 372104)
+IntegrationResolverMockTest:test_MockCancelSrc() (gas: 372184)
 IntegrationResolverMockTest:test_MockDeployDst() (gas: 162086)
-IntegrationResolverMockTest:test_MockDeploySrc() (gas: 381125)
-IntegrationResolverMockTest:test_MockPublicCancelSrc() (gas: 416126)
+IntegrationResolverMockTest:test_MockDeploySrc() (gas: 381205)
+IntegrationResolverMockTest:test_MockPublicCancelSrc() (gas: 416206)
 IntegrationResolverMockTest:test_MockPublicWithdrawDst() (gas: 247094)
 IntegrationResolverMockTest:test_MockRescueFundsDst() (gas: 174407)
-IntegrationResolverMockTest:test_MockRescueFundsSrc() (gas: 401592)
+IntegrationResolverMockTest:test_MockRescueFundsSrc() (gas: 401672)
 IntegrationResolverMockTest:test_MockWithdrawDst() (gas: 259529)
-IntegrationResolverMockTest:test_MockWithdrawToSrc() (gas: 372889)
-MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllExtra() (gas: 946191)
-MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllFromLast() (gas: 944854)
-MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllTwoFills() (gas: 944662)
-MerkleStorageInvalidatorIntTest:test_MultipleFillsFillFirst() (gas: 712231)
-MerkleStorageInvalidatorIntTest:test_MultipleFillsFillFirstTwoFills() (gas: 945481)
-MerkleStorageInvalidatorIntTest:test_MultipleFillsFillLast() (gas: 711825)
-MerkleStorageInvalidatorIntTest:test_MultipleFillsNoDeploymentWithoutValidation() (gas: 312112)
-MerkleStorageInvalidatorIntTest:test_MultipleFillsNoReuseOfSecrets() (gas: 1088778)
-MerkleStorageInvalidatorIntTest:test_MultipleFillsNoSecondDeploymentWithTheSameIndex() (gas: 804290)
-MerkleStorageInvalidatorIntTest:test_MultipleFillsOddDivision() (gas: 449479)
-MerkleStorageInvalidatorIntTest:test_MultipleFillsOneFill() (gas: 712271)
-MerkleStorageInvalidatorIntTest:test_MultipleFillsTwoFills() (gas: 943677)
+IntegrationResolverMockTest:test_MockWithdrawToSrc() (gas: 372969)
+MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllExtra() (gas: 946351)
+MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllFromLast() (gas: 945014)
+MerkleStorageInvalidatorIntTest:test_MultipleFillsFillAllTwoFills() (gas: 944822)
+MerkleStorageInvalidatorIntTest:test_MultipleFillsFillFirst() (gas: 712311)
+MerkleStorageInvalidatorIntTest:test_MultipleFillsFillFirstTwoFills() (gas: 945641)
+MerkleStorageInvalidatorIntTest:test_MultipleFillsFillLast() (gas: 711905)
+MerkleStorageInvalidatorIntTest:test_MultipleFillsNoDeploymentWithoutValidation() (gas: 312192)
+MerkleStorageInvalidatorIntTest:test_MultipleFillsNoReuseOfSecrets() (gas: 1088938)
+MerkleStorageInvalidatorIntTest:test_MultipleFillsNoSecondDeploymentWithTheSameIndex() (gas: 804450)
+MerkleStorageInvalidatorIntTest:test_MultipleFillsOddDivision() (gas: 449559)
+MerkleStorageInvalidatorIntTest:test_MultipleFillsOneFill() (gas: 712351)
+MerkleStorageInvalidatorIntTest:test_MultipleFillsTwoFills() (gas: 943837)
 MerkleStorageInvalidatorTest:test_ShortenedProofIsNotValid() (gas: 237658)
 TimelocksLibTest:test_NoTimelocksOverflow() (gas: 264056)
 TimelocksLibTest:test_getStartTimestamps() (gas: 19881)

contracts/BaseEscrow.sol

@@ -35,8 +35,8 @@ abstract contract BaseEscrow is IBaseEscrow {
         _ACCESS_TOKEN = accessToken;
     }
 
-    modifier onlyTaker(address taker) {
-        if (msg.sender != taker) revert InvalidCaller();
+    modifier onlyCaller(address expected) {
+        if (msg.sender != expected) revert InvalidCaller();
         _;
     }
 
@@ -70,7 +70,7 @@ abstract contract BaseEscrow is IBaseEscrow {
      */
     function rescueFunds(address token, uint256 amount, Immutables calldata immutables)
         external
-        onlyTaker(immutables.taker.get())
+        onlyCaller(immutables.taker.get())
         onlyValidImmutables(immutables.hash())
         onlyAfter(immutables.timelocks.rescueStart(RESCUE_DELAY))
     {

contracts/BaseEscrowFactory.sol

@@ -35,6 +35,8 @@ abstract contract BaseEscrowFactory is IEscrowFactory, SimpleSettlement, MerkleS
     using SafeERC20 for IERC20;
     using TimelocksLib for Timelocks;
 
+    error InvalidFeeAmounts();
+
     /// @notice See {IEscrowFactory-ESCROW_SRC_IMPLEMENTATION}.
     address public immutable ESCROW_SRC_IMPLEMENTATION;
     /// @notice See {IEscrowFactory-ESCROW_DST_IMPLEMENTATION}.
@@ -87,6 +89,8 @@ abstract contract BaseEscrowFactory is IEscrowFactory, SimpleSettlement, MerkleS
             extraData[:superArgsLength]
         );
 
+        if (integratorFeeAmount + protocolFeeAmount >= takingAmount) revert InvalidFeeAmounts();
+
         if (tail.length > 19) {
             IPostInteraction(address(bytes20(tail))).postInteraction(
                 order,

contracts/EscrowDst.sol

@@ -35,7 +35,7 @@ contract EscrowDst is Escrow, IEscrowDst {
      */
     function withdraw(bytes32 secret, Immutables calldata immutables)
         external
-        onlyTaker(immutables.taker.get())
+        onlyCaller(immutables.taker.get())
         onlyAfter(immutables.timelocks.get(TimelocksLib.Stage.DstWithdrawal))
         onlyBefore(immutables.timelocks.get(TimelocksLib.Stage.DstCancellation))
     {
@@ -63,7 +63,7 @@ contract EscrowDst is Escrow, IEscrowDst {
      */
     function cancel(Immutables calldata immutables)
         external
-        onlyTaker(immutables.taker.get())
+        onlyCaller(immutables.taker.get())
         onlyValidImmutables(immutables.hash())
         onlyAfter(immutables.timelocks.get(TimelocksLib.Stage.DstCancellation))
     {

contracts/EscrowSrc.sol

@@ -37,7 +37,7 @@ contract EscrowSrc is Escrow, IEscrowSrc {
      */
     function withdraw(bytes32 secret, Immutables calldata immutables)
         external
-        onlyTaker(immutables.taker.get())
+        onlyCaller(immutables.taker.get())
         onlyAfter(immutables.timelocks.get(TimelocksLib.Stage.SrcWithdrawal))
         onlyBefore(immutables.timelocks.get(TimelocksLib.Stage.SrcCancellation))
     {
@@ -52,7 +52,7 @@ contract EscrowSrc is Escrow, IEscrowSrc {
      */
     function withdrawTo(bytes32 secret, address target, Immutables calldata immutables)
         external
-        onlyTaker(immutables.taker.get())
+        onlyCaller(immutables.taker.get())
         onlyAfter(immutables.timelocks.get(TimelocksLib.Stage.SrcWithdrawal))
         onlyBefore(immutables.timelocks.get(TimelocksLib.Stage.SrcCancellation))
     {
@@ -82,7 +82,7 @@ contract EscrowSrc is Escrow, IEscrowSrc {
      */
     function cancel(Immutables calldata immutables)
         external
-        onlyTaker(immutables.taker.get())
+        onlyCaller(immutables.taker.get())
         onlyAfter(immutables.timelocks.get(TimelocksLib.Stage.SrcCancellation))
     {
         _cancel(immutables);

contracts/libraries/ImmutablesLib.sol

@@ -16,6 +16,11 @@ library ImmutablesLib {
     uint256 internal constant IMMUTABLES_SIZE = 0x120;
     uint256 internal constant IMMUTABLES_LAST_WORD = 0x100;
 
+    /**
+     * @notice Returns the protocol fee amount from the immutables.
+     * @param immutables The immutables to extract the fee from.
+     * @return ret The protocol fee amount.
+     */
     function protocolFeeAmount(IBaseEscrow.Immutables memory immutables) internal pure returns (uint256 ret) {
         bytes memory parameters = immutables.parameters;
         if (parameters.length < 0x20) revert IndexOutOfRange();
@@ -24,6 +29,11 @@ library ImmutablesLib {
         }
     }
 
+    /**
+     * @notice Returns the integrator fee amount from the immutables.
+     * @param immutables The immutables to extract the fee from.
+     * @return ret The integrator fee amount.
+     */
     function integratorFeeAmount(IBaseEscrow.Immutables memory immutables) internal pure returns (uint256 ret) {
         bytes memory parameters = immutables.parameters;
         if (parameters.length < 0x40) revert IndexOutOfRange();
@@ -32,6 +42,11 @@ library ImmutablesLib {
         }
     }
 
+    /**
+     * @notice Returns the protocol fee recipient from the immutables.
+     * @param immutables The immutables to extract the recipient from.
+     * @return ret The protocol fee recipient.
+     */
     function protocolFeeRecipient(IBaseEscrow.Immutables memory immutables) internal pure returns (Address ret) {
         bytes memory parameters = immutables.parameters;
         if (parameters.length < 0x60) revert IndexOutOfRange();
@@ -40,6 +55,11 @@ library ImmutablesLib {
         }
     }
 
+    /**
+     * @notice Returns the integrator fee recipient from the immutables.
+     * @param immutables The immutables to extract the recipient from.
+     * @return ret The integrator fee recipient.
+     */
     function integratorFeeRecipient(IBaseEscrow.Immutables memory immutables) internal pure returns (Address ret) {
         bytes memory parameters = immutables.parameters;
         if (parameters.length < 0x80) revert IndexOutOfRange();
@@ -48,6 +68,11 @@ library ImmutablesLib {
         }
     }
 
+    /**
+     * @notice Returns the protocol fee amount from the immutables (calldata version).
+     * @param immutables The immutables to extract the fee from.
+     * @return ret The protocol fee amount.
+     */
     function protocolFeeAmountCd(IBaseEscrow.Immutables calldata immutables) external pure returns (uint256 ret) {
         bytes calldata parameters = immutables.parameters;
         if (parameters.length < 0x20) revert IndexOutOfRange();
@@ -56,6 +81,11 @@ library ImmutablesLib {
         }
     }
 
+    /**
+     * @notice Returns the integrator fee amount from the immutables (calldata version).
+     * @param immutables The immutables to extract the fee from.
+     * @return ret The integrator fee amount.
+     */
     function integratorFeeAmountCd(IBaseEscrow.Immutables calldata immutables) external pure returns (uint256 ret) {
         bytes calldata parameters = immutables.parameters;
         if (parameters.length < 0x40) revert IndexOutOfRange();
@@ -64,6 +94,11 @@ library ImmutablesLib {
         }
     }
 
+    /**
+     * @notice Returns the protocol fee recipient from the immutables (calldata version).
+     * @param immutables The immutables to extract the recipient from.
+     * @return ret The protocol fee recipient.
+     */
     function protocolFeeRecipientCd(IBaseEscrow.Immutables calldata immutables) external pure returns (Address ret) {
         bytes calldata parameters = immutables.parameters;
         if (parameters.length < 0x60) revert IndexOutOfRange();
@@ -72,6 +107,11 @@ library ImmutablesLib {
         }
     }
 
+    /**
+     * @notice Returns the integrator fee recipient from the immutables (calldata version).
+     * @param immutables The immutables to extract the recipient from.
+     * @return ret The integrator fee recipient.
+     */
     function integratorFeeRecipientCd(IBaseEscrow.Immutables calldata immutables) external pure returns (Address ret) {
         bytes calldata parameters = immutables.parameters;
         if (parameters.length < 0x80) revert IndexOutOfRange();

foundry.lock

@@ -0,0 +1,20 @@
+{
+  "lib/murky": {
+    "rev": "991e371eb1dfa9f86701869eb08ec4e98c3cc0b0"
+  },
+  "lib/limit-order-settlement": {
+    "rev": "78a5a68f4814361332e347071513b3d191a630e1"
+  },
+  "lib/openzeppelin-contracts": {
+    "rev": "6ef73e33866527291e81cf98ddd1ec8b60d7aac8"
+  },
+  "lib/solidity-utils": {
+    "rev": "94611cc4ddb6c7c55f9d20f3093074ecca5440ba"
+  },
+  "lib/limit-order-protocol": {
+    "rev": "b655e74665559450ebf3500c2149b805db8ee229"
+  },
+  "lib/forge-std": {
+    "rev": "60acb7aaadcce2d68e52986a0a66fe79f07d138f"
+  }
+}

test/integration/EscrowFactory.t.sol

@@ -30,7 +30,6 @@ contract IntegrationEscrowFactoryTest is BaseSetup {
     /* solhint-disable func-name-mixedcase */
 
     function testFuzz_DeployCloneForMakerInt(bytes32 secret, uint56 srcAmount, uint56 dstAmount) public {
-        vm.skip(true);
         vm.assume(srcAmount > 0 && dstAmount > 0);
         uint256 srcSafetyDeposit = uint256(srcAmount) * 10 / 100;
         uint256 dstSafetyDeposit = uint256(dstAmount) * 10 / 100;