Merge pull request #52 from KyberNetwork/hexens_audit_iteration_2

fix: updates from Hexens audit

Author: qcuong98

src/KSSmartIntentHasher.sol

@@ -2,14 +2,21 @@
 pragma solidity ^0.8.0;
 
 import {ActionData} from './types/ActionData.sol';
+import {ActionWitnessLibrary} from './types/ActionWitness.sol';
 import {IntentData} from './types/IntentData.sol';
 
 contract KSSmartIntentHasher {
-  function hash(IntentData calldata intentData) public pure returns (bytes32) {
+  function hashIntentData(IntentData calldata intentData) public pure returns (bytes32) {
     return intentData.hash();
   }
 
-  function hash(ActionData calldata actionData) public pure returns (bytes32) {
-    return actionData.hash();
+  function hashActionWitness(bytes32 intentHash, ActionData calldata actionData)
+    public
+    pure
+    returns (bytes32)
+  {
+    return keccak256(
+      abi.encode(ActionWitnessLibrary.ACTION_WITNESS_TYPE_HASH, intentHash, actionData.hash())
+    );
   }
 }

src/KSSmartIntentRouter.sol

@@ -6,13 +6,14 @@ import {KSSmartIntentRouterAccounting} from './KSSmartIntentRouterAccounting.sol
 import {KSSmartIntentRouterNonces} from './KSSmartIntentRouterNonces.sol';
 
 import {IKSSmartIntentRouter} from './interfaces/IKSSmartIntentRouter.sol';
+import {IKSAllowanceHub} from './interfaces/actions/IKSAllowanceHub.sol';
 import {IKSSwapRouterV2} from './interfaces/actions/IKSSwapRouterV2.sol';
 import {IKSSwapRouterV3} from './interfaces/actions/IKSSwapRouterV3.sol';
 import {IKSZapRouter} from './interfaces/actions/IKSZapRouter.sol';
-
 import {HookLibrary} from './libraries/HookLibrary.sol';
 
 import {ActionData} from './types/ActionData.sol';
+import {ActionWitness} from './types/ActionWitness.sol';
 import {IntentCoreData} from './types/IntentCoreData.sol';
 import {IntentData} from './types/IntentData.sol';
 
@@ -78,7 +79,7 @@ contract KSSmartIntentRouter is
       revert NotMainAddress();
     }
 
-    _delegate(intentData, _hashTypedDataV4(hasher.hash(intentData)));
+    _delegate(intentData, _hashTypedDataV4(hasher.hashIntentData(intentData)));
   }
 
   /// @inheritdoc IKSSmartIntentRouter
@@ -87,7 +88,7 @@ contract KSSmartIntentRouter is
       revert NotMainAddress();
     }
 
-    bytes32 intentHash = _hashTypedDataV4(hasher.hash(intentData));
+    bytes32 intentHash = _hashTypedDataV4(hasher.hashIntentData(intentData));
     intentStatuses[intentHash] = IntentStatus.REVOKED;
 
     emit RevokeIntent(intentHash);
@@ -101,7 +102,7 @@ contract KSSmartIntentRouter is
     bytes calldata gdSignature,
     ActionData calldata actionData
   ) public {
-    bytes32 intentHash = _hashTypedDataV4(hasher.hash(intentData));
+    bytes32 intentHash = _hashTypedDataV4(hasher.hashIntentData(intentData));
     _execute(intentHash, intentData, dkSignature, guardian, gdSignature, actionData);
   }
 
@@ -114,7 +115,7 @@ contract KSSmartIntentRouter is
     bytes calldata gdSignature,
     ActionData calldata actionData
   ) public {
-    bytes32 intentHash = _hashTypedDataV4(hasher.hash(intentData));
+    bytes32 intentHash = _hashTypedDataV4(hasher.hashIntentData(intentData));
     if (!intentData.coreData.mainAddress.isValidSignatureNowCalldata(intentHash, maSignature)) {
       revert InvalidMainAddressSignature();
     }
@@ -164,11 +165,7 @@ contract KSSmartIntentRouter is
     _useUnorderedNonce(intentHash, actionData.nonce);
 
     _validateActionData(
-      intentData.coreData,
-      dkSignature,
-      guardian,
-      gdSignature,
-      _hashTypedDataV4(hasher.hash(actionData))
+      intentHash, intentData.coreData, actionData, dkSignature, guardian, gdSignature
     );
 
     (uint256[] memory fees, bytes memory beforeExecutionData) =
@@ -212,40 +209,44 @@ contract KSSmartIntentRouter is
       selector == IKSSwapRouterV2.swap.selector
         || selector == IKSSwapRouterV2.swapSimpleMode.selector
         || selector == IKSSwapRouterV3.swap.selector || selector == IKSZapRouter.zap.selector
+        || selector == IKSAllowanceHub.permitTransferAndExecute.selector
     ) {
       return IKSGenericForwarder(address(0));
     }
     return forwarder;
   }
 
   function _validateActionData(
+    bytes32 intentHash,
     IntentCoreData calldata coreData,
+    ActionData calldata actionData,
     bytes calldata dkSignature,
     address guardian,
-    bytes calldata gdSignature,
-    bytes32 actionHash
+    bytes calldata gdSignature
   ) internal view {
+    bytes32 witnessHash = _hashTypedDataV4(hasher.hashActionWitness(intentHash, actionData));
+
     if (coreData.signatureVerifier == address(0)) {
       /// @dev use ECDSA scheme
       address delegatedAddress = coreData.delegatedKey.decodeAddress();
       if (
         msg.sender != delegatedAddress
-          && !delegatedAddress.isValidSignatureNowCalldata(actionHash, dkSignature)
+          && !delegatedAddress.isValidSignatureNowCalldata(witnessHash, dkSignature)
       ) {
         revert InvalidDelegatedKeySignature();
       }
     } else {
       if (
         IERC7913SignatureVerifier(coreData.signatureVerifier)
-            .verify(coreData.delegatedKey, actionHash, dkSignature)
+            .verify(coreData.delegatedKey, witnessHash, dkSignature)
           != IERC7913SignatureVerifier.verify.selector
       ) {
         revert InvalidDelegatedKeySignature();
       }
     }
 
     if (msg.sender != guardian) {
-      if (!guardian.isValidSignatureNowCalldata(actionHash, gdSignature)) {
+      if (!guardian.isValidSignatureNowCalldata(witnessHash, gdSignature)) {
         revert InvalidGuardianSignature();
       }
     }

src/hooks/base/BaseTickBasedRemoveLiquidityHook.sol

@@ -22,6 +22,7 @@ abstract contract BaseTickBasedRemoveLiquidityHook is BaseConditionalHook {
   error InvalidLiquidity();
   error NotEnoughOutputAmount();
   error NotEnoughFeesReceived();
+  error ExceedMaxFeesPercent();
 
   uint256 public constant Q128 = 1 << 128;
   address public immutable WETH;
@@ -255,16 +256,29 @@ abstract contract BaseTickBasedRemoveLiquidityHook is BaseConditionalHook {
       validationData.maxFees[removeLiqParams.index] >> 128,
       uint128(validationData.maxFees[removeLiqParams.index])
     ];
+
+    require(
+      outputParams.intentFeesPercent[0] <= outputParams.maxFees[0]
+        && outputParams.intentFeesPercent[1] <= outputParams.maxFees[1],
+      ExceedMaxFeesPercent()
+    );
   }
 
+  /**
+   * @notice Validate the conditions of the liquidity removal operation
+   * @param nodes The nodes of conditions (used to build the condition tree)
+   * @param fee0Generated The fee0 generated - an offchain component that could contain claimed fees, unclaimed fees, and yield-based fees
+   * @param fee1Generated The fee1 generated - an offchain component that could contain claimed fees, unclaimed fees, and yield-based fees
+   * @param poolPrice The price of the pool
+   */
   function _validateConditions(
     Node[] calldata nodes,
-    uint256 fee0Collected,
-    uint256 fee1Collected,
+    uint256 fee0Generated,
+    uint256 fee1Generated,
     uint256 poolPrice
   ) internal view virtual {
     this.validateConditionTree(
-      _buildConditionTree(nodes, fee0Collected, fee1Collected, poolPrice), 0
+      _buildConditionTree(nodes, fee0Generated, fee1Generated, poolPrice), 0
     );
   }
 
@@ -318,8 +332,8 @@ abstract contract BaseTickBasedRemoveLiquidityHook is BaseConditionalHook {
 
   function _buildConditionTree(
     Node[] calldata nodes,
-    uint256 fee0Collected,
-    uint256 fee1Collected,
+    uint256 fee0Generated,
+    uint256 fee1Generated,
     uint256 poolPrice
   ) internal pure virtual returns (ConditionTree memory conditionTree) {
     conditionTree.nodes = nodes;
@@ -329,7 +343,7 @@ abstract contract BaseTickBasedRemoveLiquidityHook is BaseConditionalHook {
         continue;
       }
       if (nodes[i].condition.isType(YIELD_BASED)) {
-        conditionTree.additionalData[i] = abi.encode(fee0Collected, fee1Collected, poolPrice);
+        conditionTree.additionalData[i] = abi.encode(fee0Generated, fee1Generated, poolPrice);
       } else if (nodes[i].condition.isType(PRICE_BASED)) {
         conditionTree.additionalData[i] = abi.encode(poolPrice);
       }

src/interfaces/actions/IKSAllowanceHub.sol

@@ -0,0 +1,59 @@
+/// @title IKSAllowanceHub
+pragma solidity ^0.8.0;
+
+/// @notice Interface for the KSAllowanceHub
+interface IKSAllowanceHub {
+  /**
+   * @notice Parameters for collecting ERC20 tokens from `msg.sender`
+   * @param token The address of the tokens to collect
+   * @param targets The addresses to transfer the tokens to
+   * @param amounts The amounts to transfer to each target
+   * @param permitData The permit data for the tokens
+   */
+  struct ERC20Params {
+    address token;
+    address[] targets;
+    uint256[] amounts;
+    bytes permitData;
+  }
+
+  /**
+   * @notice Parameters for collecting an ERC721 token from `msg.sender`
+   * @param token The address of the token to collect
+   * @param tokenId The token ID to collect
+   * @param target The address to transfer the token to
+   * @param permitData The permit data for the token
+   */
+  struct ERC721Params {
+    address token;
+    uint256 tokenId;
+    address target;
+    bytes permitData;
+  }
+
+  /**
+   * @notice Parameters for calling a generic router
+   * @param router The address of the router
+   * @param value The value to send along with the call
+   * @param data The data to call the generic router with
+   */
+  struct GenericCall {
+    address router;
+    uint256 value;
+    bytes data;
+  }
+
+  /**
+   * @notice Permits, transfers ERC20 and ERC721 tokens, executes generic calls
+   * @param erc20Params The ERC20 tokens to transfer
+   * @param erc721Params The ERC721 tokens to transfer
+   * @param genericCalls The generic calls to execute
+   * @return results The results of the generic calls
+   * @return gasUsed The amount of gas used
+   */
+  function permitTransferAndExecute(
+    ERC20Params[] calldata erc20Params,
+    ERC721Params[] calldata erc721Params,
+    GenericCall[] calldata genericCalls
+  ) external payable returns (bytes[] memory results, uint256 gasUsed);
+}

src/interfaces/actions/IKSSwapRouterV3.sol

@@ -9,10 +9,12 @@ interface IKSSwapRouterV3 {
   /// @param targets The targets to transfer the input token to
   /// @param amounts The amounts to transfer to the targets
   struct InputTokenData {
-    // length = 5 * 32: IERC20Permit, use ERC20 `transferFrom`
-    // length = 6 * 32: IDaiLikePermit, use ERC20 `transferFrom`
-    // length = 0: use ERC20 `transferFrom`
-    // otherwise: use Permit2 `transferFrom`
+    // Permit method selection:
+    // length = 5 * 32: use ERC20 `permit`
+    // length = 6 * 32: use DAI `permit`
+    // Transfer method selection:
+    // length == 0: use Permit2 `transferFrom`
+    // length != 0: use ERC20 `transferFrom`
     bytes permitData;
     address[] feeRecipients;
     uint256[] fees;
@@ -31,32 +33,37 @@ interface IKSSwapRouterV3 {
   }
 
   /// @notice Contains the parameters for a swap
-  /// @param permit2Data The data to call permit2 with
   /// @param inputTokens The input tokens
-  /// @param inputAmounts The input amounts
+  /// @param inputAmounts The input amounts (only used for fee calculation)
   /// @param inputData The additional data for the input tokens
   /// @param outputTokens The output tokens
   /// @param outputData The additional data for the output tokens
+  /// @param permit2Data The data to call permit2 with
   /// @param executor The executor to call
   /// @param executorData The data to pass to the executor
   /// @param recipient The recipient of the output tokens
+  /// @param deadline The deadline for the swap
   /// @param clientData The client data
   struct SwapParams {
-    bytes permit2Data;
     address[] inputTokens;
     uint256[] inputAmounts;
     InputTokenData[] inputData;
     address[] outputTokens;
     OutputTokenData[] outputData;
+    bytes permit2Data;
     address executor;
     bytes executorData;
     address recipient;
+    uint256 deadline;
     bytes clientData;
   }
 
   /// @notice Entry point for swapping
   /// @param params The parameters for the swap
-  function swap(SwapParams calldata params) external payable;
+  function swap(SwapParams calldata params)
+    external
+    payable
+    returns (uint256[] memory outputAmounts, uint256 gasUsed);
 
   /// @notice Returns the address of who called the swap function
   function msgSender() external view returns (address);

src/types/ActionWitness.sol

@@ -0,0 +1,23 @@
+// SPDX-License-Identifier: GPL-3.0-or-later
+pragma solidity ^0.8.0;
+
+import {ActionData} from './ActionData.sol';
+
+struct ActionWitness {
+  bytes32 intentHash;
+  ActionData actionData;
+}
+
+using ActionWitnessLibrary for ActionWitness global;
+
+library ActionWitnessLibrary {
+  bytes32 constant ACTION_WITNESS_TYPE_HASH = keccak256(
+    abi.encodePacked(
+      'ActionWitness(bytes32 intentHash,ActionData actionData)ActionData(uint256[] erc20Ids,uint256[] erc20Amounts,uint256[] erc721Ids,FeeInfo feeInfo,uint256 approvalFlags,uint256 actionSelectorId,bytes actionCalldata,bytes hookActionData,bytes extraData,uint256 deadline,uint256 nonce)FeeInfo(address protocolRecipient,uint256[][] partnerFeeConfigs)'
+    )
+  );
+
+  function hash(ActionWitness calldata self) internal pure returns (bytes32) {
+    return keccak256(abi.encode(ACTION_WITNESS_TYPE_HASH, self.intentHash, self.actionData.hash()));
+  }
+}

test/Base.t.sol

@@ -127,28 +127,40 @@ contract BaseTest is Test {
     return abi.encodePacked(r, s, v);
   }
 
-  function _getGDSignature(ActionData memory actionData) internal view returns (bytes memory) {
-    bytes32 intentHash = router.hashTypedActionData(actionData);
-    (uint8 v, bytes32 r, bytes32 s) = vm.sign(guardianKey, intentHash);
+  function _getGDSignature(IntentData memory intentData, ActionData memory actionData)
+    internal
+    view
+    returns (bytes memory)
+  {
+    bytes32 witnessHash = router.hashTypedActionWitness(
+      ActionWitness(router.hashTypedIntentData(intentData), actionData)
+    );
+    (uint8 v, bytes32 r, bytes32 s) = vm.sign(guardianKey, witnessHash);
     return abi.encodePacked(r, s, v);
   }
 
-  function _getDKSignature(ActionData memory actionData) internal view returns (bytes memory) {
-    bytes32 actionHash = router.hashTypedActionData(actionData);
-    return _getSignature(delegatedPrivateKey, actionHash);
-  }
-
-  function _getCallerAndSignatures(uint256 mode, ActionData memory actionData)
+  function _getDKSignature(IntentData memory intentData, ActionData memory actionData)
     internal
     view
-    returns (address caller, bytes memory dkSignature, bytes memory gdSignature)
+    returns (bytes memory)
   {
+    bytes32 witnessHash = router.hashTypedActionWitness(
+      ActionWitness(router.hashTypedIntentData(intentData), actionData)
+    );
+    return _getSignature(delegatedPrivateKey, witnessHash);
+  }
+
+  function _getCallerAndSignatures(
+    uint256 mode,
+    IntentData memory intentData,
+    ActionData memory actionData
+  ) internal view returns (address caller, bytes memory dkSignature, bytes memory gdSignature) {
     caller = mode == 0 ? randomCaller : (mode == 1 ? guardian : vm.addr(delegatedPrivateKey));
     if (mode == 0 || mode == 1) {
-      dkSignature = _getDKSignature(actionData);
+      dkSignature = _getDKSignature(intentData, actionData);
     }
     if (mode == 0 || mode == 2) {
-      gdSignature = _getGDSignature(actionData);
+      gdSignature = _getGDSignature(intentData, actionData);
     }
   }