From e5fd59d5b5f4b2462d9cc09468ddbcfdc5b6843a Mon Sep 17 00:00:00 2001 From: JaredBorders Date: Tue, 5 Sep 2023 12:34:02 -0400 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=91=B7=F0=9F=8F=BB=E2=80=8D=E2=99=82?= =?UTF-8?q?=EF=B8=8F=20Update=20Engine.verifyConditions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/Engine.sol | 11 ++++++++--- src/interfaces/IEngine.sol | 8 +++++++- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/Engine.sol b/src/Engine.sol index 2a656760..513b4ca2 100644 --- 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 index b8e24423..dbd6c7e3 100644 --- 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); /*////////////////////////////////////////////////////////////// From ed4d817fe60382734bae593c394b1d42629217ef Mon Sep 17 00:00:00 2001 From: JaredBorders Date: Tue, 5 Sep 2023 12:34:40 -0400 Subject: [PATCH 2/2] =?UTF-8?q?=E2=9C=85=20Add=20verifyConditions=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- test/ConditionalOrder.t.sol | 91 +++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/test/ConditionalOrder.t.sol b/test/ConditionalOrder.t.sol index 63baaeea..7f3f130e 100644 --- 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 {