👷🏻‍♂️ Improve verifyConditions

Kwenta · Sep 5, 2023 · 99f3dc5 · 99f3dc5
2 parents 6ce117e + ed4d817
commit 99f3dc5
Show file tree

Hide file tree

Showing 3 changed files with 106 additions and 4 deletions.
diff --git a/src/Engine.sol b/src/Engine.sol
@@ -389,7 +389,7 @@ contract Engine is IEngine, Multicallable, EIP712, ERC721Receivable {
     function canExecute(
         ConditionalOrder calldata _co,
         bytes calldata _signature
-    ) public override returns (bool) {
+    ) public view override returns (bool) {
         // verify nonce has not been executed before
         if (executedOrders[_co.nonce]) return false;
 
@@ -449,14 +449,19 @@ contract Engine is IEngine, Multicallable, EIP712, ERC721Receivable {
     /// @inheritdoc IEngine
     function verifyConditions(ConditionalOrder calldata _co)
         public
+        view
         override
         returns (bool)
     {
         uint256 length = _co.conditions.length;
         if (length > MAX_CONDITIONS) revert MaxConditionSizeExceeded();
         for (uint256 i = 0; i < length;) {
-            (bool success, bytes memory response) =
-                address(this).call(_co.conditions[i]);
+            bool success;
+            bytes memory response;
+
+            /// @dev staticcall to prevent state changes in the case a condition is malicious
+            (success, response) = address(this).staticcall(_co.conditions[i]);
+
             if (!success || !abi.decode(response, (bool))) return false;
 
             unchecked {

diff --git a/src/interfaces/IEngine.sol b/src/interfaces/IEngine.sol
@@ -181,7 +181,7 @@ interface IEngine {
     function canExecute(
         ConditionalOrder calldata _co,
         bytes calldata _signature
-    ) external returns (bool);
+    ) external view returns (bool);
 
     /// @notice verify the conditional order signer is the owner or delegate of the account
     /// @param _co the conditional order
@@ -201,10 +201,16 @@ interface IEngine {
     ) external view returns (bool);
 
     /// @notice verify array of conditions defined in the conditional order
+    /// @dev 
+    ///     1. all conditions are defined by the conditional order creator
+    ///     2. conditions are encoded function selectors and parameters
+    ///     3. each function defined in the condition contract must return a truthy value
+    ///     4. internally, staticcall is used to protect against malicious conditions
     /// @param _co the conditional order
     /// @return true if all conditions are met
     function verifyConditions(ConditionalOrder calldata _co)
         external
+        view
         returns (bool);
 
     /*//////////////////////////////////////////////////////////////

diff --git a/test/ConditionalOrder.t.sol b/test/ConditionalOrder.t.sol
@@ -364,6 +364,97 @@ contract VerifyConditions is ConditionalOrderTest {
 
         assertFalse(isVerified);
     }
+
+    function test_verifyConditions_public_non_condition_isAccountDelegate()
+        public
+    {
+        bytes[] memory conditions = new bytes[](1);
+        conditions[0] = abi.encodeWithSelector(
+            IEngine.isAccountDelegate.selector, accountId, address(engine)
+        );
+
+        IEngine.OrderDetails memory orderDetails;
+
+        IEngine.ConditionalOrder memory co = IEngine.ConditionalOrder({
+            orderDetails: orderDetails,
+            signer: signer,
+            nonce: 0,
+            requireVerified: true,
+            trustedExecutor: address(0),
+            conditions: conditions
+        });
+
+        bool isVerified = engine.verifyConditions(co);
+
+        assertTrue(isVerified);
+    }
+
+    function test_verifyConditions_external_non_condition_getAccountStats()
+        public
+    {
+        bytes[] memory conditions = new bytes[](1);
+        conditions[0] =
+            abi.encodeWithSelector(IEngine.getAccountStats.selector, accountId);
+
+        IEngine.OrderDetails memory orderDetails;
+
+        IEngine.ConditionalOrder memory co = IEngine.ConditionalOrder({
+            orderDetails: orderDetails,
+            signer: signer,
+            nonce: 0,
+            requireVerified: true,
+            trustedExecutor: address(0),
+            conditions: conditions
+        });
+
+        bool isVerified = engine.verifyConditions(co);
+
+        assertFalse(isVerified);
+    }
+
+    function test_verifyConditions_internal_non_condition_getSynthAddress()
+        public
+    {
+        bytes[] memory conditions = new bytes[](1);
+        conditions[0] = abi.encodeWithSignature(
+            "_getSynthAddress(uint128 _synthMarketId)", SETH_SPOT_MARKET_ID
+        );
+
+        IEngine.OrderDetails memory orderDetails;
+
+        IEngine.ConditionalOrder memory co = IEngine.ConditionalOrder({
+            orderDetails: orderDetails,
+            signer: signer,
+            nonce: 0,
+            requireVerified: true,
+            trustedExecutor: address(0),
+            conditions: conditions
+        });
+
+        bool isVerified = engine.verifyConditions(co);
+
+        assertFalse(isVerified);
+    }
+
+    function test_verifyConditions_modify_state_createAccount() public {
+        bytes[] memory conditions = new bytes[](1);
+        conditions[0] = abi.encodeWithSignature("createAccount()");
+
+        IEngine.OrderDetails memory orderDetails;
+
+        IEngine.ConditionalOrder memory co = IEngine.ConditionalOrder({
+            orderDetails: orderDetails,
+            signer: signer,
+            nonce: 0,
+            requireVerified: true,
+            trustedExecutor: address(0),
+            conditions: conditions
+        });
+
+        bool isVerified = engine.verifyConditions(co);
+
+        assertFalse(isVerified);
+    }
 }
 
 contract Execute is ConditionalOrderTest {