Feat/operator restrictions #4

0xhammadghazi · 2023-10-16T20:50:18Z

Stubs implemented:

Only operator restriction.
Order-specific sanity checks.
Verify signatures.
Call pre-interaction hook.
Transfer funds from the maker to the vault.
Call facilitator interaction.
Ensure the facilitator is respecting the maker's limit price.
Transfer funds from the vault to the recipient.
Call post-interaction hook.
Log events.

Removed 'signature' member from 'Order' struct as it's essentially a message that users will sign. Having signature in the message makes the contract susceptible to signature malleability attacks.

…and define constructor

implemented vault contract to facilitate asset transfers between the maker and the vault.

integrated vault contract for asset transfers from the maker's address to the vault contract in AdvancedOrderEngine contract. Imported vault contract and invoked its _receiveAsset function for funds transfer

Added a validation check to ensure that maker receives an amount equal to or more than requested

…nt in vault-to-maker asset transfer

…unts'

…tion hook

removed the 'facilitatorInteractionTargetContract' var from the 'fillOrders' fn param. Facilitator is now expected to provide the target contract address within the 'facilitatorInteractionCallData' param. Also removed the facilitator target contract address check

…fn param to 'facilitatorInteraction'

…face

0xabdullah0 · 2023-10-17T07:57:55Z

src/libraries/OrderEngine.sol

+    bytes32 public constant ORDER_TYPE_HASH =
+        keccak256(
+            "Order("
+            "uint256 nonce,"


Can we use smaller datatype for nonce ?
@0xhammadghazi

Yes, I'm considering this. Actually, we can consider using smaller types for the following variables: nonce, validTill, sellTokenAmount, buyTokenAmount, and feeAmounts

gul-hameed · 2023-10-17T08:23:41Z

src/AdvancedOrderEngine.sol

@@ -33,6 +31,13 @@ contract AdvancedOrderEngine is Vault, EIP712 {
        string memory version
    ) EIP712(name, version) {}

+    modifier onlyOperator() {
+        if (!isOperator[msg.sender]) {


@0xhammadghazi Refactor a modifier to call a local function instead of directly having the code in the modifier, saving bytecode size and thereby deployment cost Ref

Acknowledged. The bytecode of the function modifier is duplicated/copied to each method, but we are currently using onlyOperator in only one function. Therefore, it won't make any difference at the moment, but yes, it's a good point to consider during the gas optimization phase.

0xabdullah0 · 2023-10-18T06:56:47Z

src/AdvancedOrderEngine.sol

+            // STUB: PARTIAL FILL FEATURE //
+
+            // TBD: debatable, can take signing scheme in order schema or can verify like 1inch
+            if (order.isContract()) {


Can we make seperate function for processing orders from contract or account like 1inch is doing , better for readibility..
https://github.com/1inch/limit-order-protocol/blob/master/contracts/OrderMixin.sol#L222C15-L222C15

@0xhammadghazi

Okay, I will take it into consideration.

0xabdullah0 · 2023-10-18T07:01:18Z

src/AdvancedOrderEngine.sol

+            }
+
+            // STUB: VERIFY PREDICATES //
+


@0xhammadghazi Will there be any more checks related to predicates ?

I'm not sure yet, I will work on the predicates section tomorrow, so only then can I tell.

gul-hameed · 2023-10-18T08:19:02Z

src/AdvancedOrderEngine.sol

+        isOperator[_address] = _access;
+
+        emit OperatorAccessModified(_address, _access);
+    }

    /**
     * @notice Fills multiple orders by processing the specified orders and clearing prices.


@0xhammadghazi
The natspec for signatures seems missing.

acknowledged

gul-hameed · 2023-10-18T08:20:09Z

src/AdvancedOrderEngine.sol

@@ -34,30 +88,137 @@ contract AdvancedOrderEngine {
        }

        // Revert if the length of the orders array does not match the clearing prices array.
-        if (orders.length != clearingPrices.length) {
-            revert ArraysLengthMismatch(orders.length, clearingPrices.length);
+        if (orders.length != offeredAmounts.length) {


@0xhammadghazi
do you think that we also check that signatures.length matches orders.length?

gul-hameed · 2023-12-04T11:05:11Z

@HareemSaad

According to this PR I've gone through the Slither report and here's what stands out:
slither.md

Calls-Loop in fillOrders: We've got external calls inside a loop which can hike up gas costs and might open doors for reentrancy bugs. How about we try simulating the orders before going all in? This could give us a better handle on what's executable.
Cyclomatic-Complexity: Our fillOrders function is a bit complex, which could make it tricky to manage and test. Maybe breaking it down could help? Like pulling out the signature verification into its own thing could simplify stuff a lot.

Once we tackle these and any other points you spot in the PR, we should be good to merge!

Hammad Ghazi added 30 commits October 11, 2023 17:34

implemented order specific sanity checks

6a9cd63

fix: order schema

305fe99

Removed 'signature' member from 'Order' struct as it's essentially a message that users will sign. Having signature in the message makes the contract susceptible to signature malleability attacks.

feat: Import EIP712 and EIP1271 contracts for signature verification …

74b3fe4

…and define constructor

feat: implement order_type_hash for signature verification

f7620c6

feat: implement and use order hash fns for signature verification

f514692

fix: add signatures param in fillOrder fn

9650e28

feat: implement signature verification core logic

c77178c

fix: stack too deep by setting via_ir to true

e2bf7f6

feat: implement fn to decode calldata

de85df4

define pre-interaction hook interface

8009baa

feat: import and attach decoder lib for hooks

a396f4c

feat: implement core logic of pre-interaction hook

9aaac8a

fix: modify stubs as comments

f762069

fix: add another stub as a comment

410a120

feat: implement vault contract

2f7baa3

implemented vault contract to facilitate asset transfers between the maker and the vault.

feat: integrate vault contract for maker-to-vault asset transfer

a419357

integrated vault contract for asset transfers from the maker's address to the vault contract in AdvancedOrderEngine contract. Imported vault contract and invoked its _receiveAsset function for funds transfer

feat: implement logic for vault-to-maker asset transfer

99211c4

feat: define post-interaction hook interface

aa21e29

feat: implement core logic of post-interaction hook

1646350

feat: add validation for maker requested amount

c8ede00

Added a validation check to ensure that maker receives an amount equal to or more than requested

fix: corrected transfer from offered amount instead of requested amou…

a9af474

…nt in vault-to-maker asset transfer

refacator: renamed 'clearingPrices' fillOrder fn param to 'offeredAmo…

8821760

…unts'

fix: passing 'orderMessageHash' instead of 'orderHash' in pre-interac…

44d6b9f

…tion hook

feat: log OrderFill event

edee642

define facilitator interaction hook interface

5479a53

refactor: renamed 'facilitatorInteractionCalldata' var of fillOrders …

583ae79

…fn param to 'facilitatorInteraction'

refactor: modified facilitator interaction hook interface

3010816

feat: implement core logic of facilitator interaction hook

9ed479a

docs: modified natspec comments of facilitator interaction hook inter…

52a9bea

…face

docs: added some TBD comments

d2a300a

0xhammadghazi requested review from gul-hameed, 0xabdullah0 and imxm and removed request for gul-hameed and 0xabdullah0 October 16, 2023 20:59

0xabdullah0 reviewed Oct 17, 2023

View reviewed changes

gul-hameed requested review from gul-hameed and removed request for imxm October 17, 2023 10:31

gul-hameed reviewed Oct 17, 2023

View reviewed changes

BlockApex deleted a comment from gul-hameed Oct 17, 2023

0xabdullah0 reviewed Oct 18, 2023

View reviewed changes

gul-hameed reviewed Oct 18, 2023

View reviewed changes

gul-hameed added 12 commits October 19, 2023 16:00

Create slither.yml

efe5a3b

Update slither.yml

48d19f2

Update slither.yml

caa07f0

Update slither.yml

564a5e4

Update slither.yml

20e8888

Update slither.yml

6a6b68f

Update slither.yml

caf8f71

Update slither.yml

6af3d10

Update slither.yml

31880f4

Update slither.yml

ccaaaf0

Create slither-output.txt

4508197

Update slither.yml

c4921e1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/operator restrictions #4

Feat/operator restrictions #4

0xhammadghazi commented Oct 16, 2023

0xabdullah0 Oct 17, 2023

0xhammadghazi Oct 17, 2023

gul-hameed Oct 17, 2023

0xhammadghazi Oct 19, 2023

0xabdullah0 Oct 18, 2023

0xabdullah0 Oct 18, 2023

0xhammadghazi Oct 19, 2023

0xabdullah0 Oct 18, 2023

0xhammadghazi Oct 19, 2023

gul-hameed Oct 18, 2023

0xhammadghazi Oct 19, 2023

gul-hameed Oct 18, 2023 •

edited

Loading

0xhammadghazi Oct 19, 2023

gul-hameed commented Dec 4, 2023

Feat/operator restrictions #4

Are you sure you want to change the base?

Feat/operator restrictions #4

Conversation

0xhammadghazi commented Oct 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gul-hameed Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gul-hameed commented Dec 4, 2023

gul-hameed Oct 18, 2023 •

edited

Loading