Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/operator restrictions #4

Open
wants to merge 47 commits into
base: feat/generic-sanity-checks
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
6a9cd63
implemented order specific sanity checks
Oct 11, 2023
305fe99
fix: order schema
Oct 15, 2023
74b3fe4
feat: Import EIP712 and EIP1271 contracts for signature verification …
Oct 15, 2023
f7620c6
feat: implement order_type_hash for signature verification
Oct 15, 2023
f514692
feat: implement and use order hash fns for signature verification
Oct 15, 2023
9650e28
fix: add signatures param in fillOrder fn
Oct 15, 2023
c77178c
feat: implement signature verification core logic
Oct 15, 2023
e2bf7f6
fix: stack too deep by setting via_ir to true
Oct 15, 2023
de85df4
feat: implement fn to decode calldata
Oct 15, 2023
8009baa
define pre-interaction hook interface
Oct 15, 2023
a396f4c
feat: import and attach decoder lib for hooks
Oct 15, 2023
9aaac8a
feat: implement core logic of pre-interaction hook
Oct 15, 2023
f762069
fix: modify stubs as comments
Oct 15, 2023
410a120
fix: add another stub as a comment
Oct 15, 2023
2f7baa3
feat: implement vault contract
Oct 16, 2023
a419357
feat: integrate vault contract for maker-to-vault asset transfer
Oct 16, 2023
99211c4
feat: implement logic for vault-to-maker asset transfer
Oct 16, 2023
aa21e29
feat: define post-interaction hook interface
Oct 16, 2023
1646350
feat: implement core logic of post-interaction hook
Oct 16, 2023
c8ede00
feat: add validation for maker requested amount
Oct 16, 2023
a9af474
fix: corrected transfer from offered amount instead of requested amou…
Oct 16, 2023
8821760
refacator: renamed 'clearingPrices' fillOrder fn param to 'offeredAmo…
Oct 16, 2023
44d6b9f
fix: passing 'orderMessageHash' instead of 'orderHash' in pre-interac…
Oct 16, 2023
edee642
feat: log OrderFill event
Oct 16, 2023
5479a53
define facilitator interaction hook interface
Oct 16, 2023
7ea9136
refactor: modify 'fillOrders' fn param
Oct 16, 2023
583ae79
refactor: renamed 'facilitatorInteractionCalldata' var of fillOrders …
Oct 16, 2023
3010816
refactor: modified facilitator interaction hook interface
Oct 16, 2023
9ed479a
feat: implement core logic of facilitator interaction hook
Oct 16, 2023
52a9bea
docs: modified natspec comments of facilitator interaction hook inter…
Oct 16, 2023
5d1d7e3
refactor: removed unnecessary comment
Oct 16, 2023
a09b9b1
feat: implement only operator modifier
Oct 16, 2023
dd87423
feat: implement manage operator role fn
Oct 16, 2023
5c9f6b5
fix: transfer funds to recipient from vault instead of maker
Oct 16, 2023
d2a300a
docs: added some TBD comments
Oct 16, 2023
efe5a3b
Create slither.yml
gul-hameed Oct 19, 2023
48d19f2
Update slither.yml
gul-hameed Oct 19, 2023
caa07f0
Update slither.yml
gul-hameed Oct 19, 2023
564a5e4
Update slither.yml
gul-hameed Oct 19, 2023
20e8888
Update slither.yml
gul-hameed Oct 19, 2023
6a6b68f
Update slither.yml
gul-hameed Oct 19, 2023
caf8f71
Update slither.yml
gul-hameed Oct 19, 2023
6af3d10
Update slither.yml
gul-hameed Oct 19, 2023
31880f4
Update slither.yml
gul-hameed Oct 25, 2023
ccaaaf0
Update slither.yml
gul-hameed Oct 25, 2023
4508197
Create slither-output.txt
gul-hameed Oct 25, 2023
c4921e1
Update slither.yml
gul-hameed Oct 25, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 41 additions & 0 deletions .github/workflows/slither.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
name: Slither Analysis

on: [push, pull_request]

jobs:
analyze:
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v3

- name: Run Slither
uses: crytic/[email protected]
id: slither
with:
target: 'src'
node-version: 16
fail-on: none
slither-args: --checklist --show-ignored-findings --markdown-root ${{ github.server_url }}/${{ github.repository }}/blob/${{ github.sha }}/

- name: Save Slither output to file
run: echo "${{ steps.slither.outputs.stdout }}" > slither-output.txt

- name: Configure Git
run: |
git config user.name "GitHub Actions Bot"
git config user.email "[email protected]"

- name: Commit and push changes
run: |
git add slither-output.txt
git commit -m "Update Slither analysis output"
git push

# Upload Slither Report as an Artifact
- name: Upload Slither Report
if: always() # Ensure the artifact is uploaded even if previous steps fail
uses: actions/upload-artifact@v3
with:
name: slither-report
path: slither-output.txt
1 change: 1 addition & 0 deletions foundry.toml
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ libs = ['lib']

auto_detect_solc = false
solc = '0.8.21'
via_ir = true
optimizer = true
optimizer_runs = 10_000

Expand Down
1 change: 1 addition & 0 deletions slither-output.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@

223 changes: 192 additions & 31 deletions src/AdvancedOrderEngine.sol
Original file line number Diff line number Diff line change
@@ -1,31 +1,85 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.21;

import {EIP712, ECDSA} from "openzeppelin/utils/cryptography/EIP712.sol";
import {IERC1271} from "openzeppelin/interfaces/IERC1271.sol";
import {OrderEngine} from "./libraries/OrderEngine.sol";
import {IPreInteractionNotificationReceiver} from "./interfaces/IPreInteractionNotificationReceiver.sol";
import {IPostInteractionNotificationReceiver} from "./interfaces/IPostInteractionNotificationReceiver.sol";

import {IInteractionNotificationReceiver} from "./interfaces/IInteractionNotificationReceiver.sol";

import {Decoder} from "./libraries/Decoder.sol";
import "./AdvancedOrderEngineErrors.sol";
import {Vault} from "./Vault.sol";
import {Ownable2Step} from "openzeppelin/access/Ownable2Step.sol";

contract AdvancedOrderEngine is Vault, Ownable2Step, EIP712 {
// TBD: consider making extraData a separate param
// TBD: consider making interfaces generic
// TBD: consider changing data type to IERC20 of buy and sell token
// TBD: consider allowing facilitator to tell offeredAmounts in its interaction

contract AdvancedOrderEngine {
using OrderEngine for OrderEngine.Order;
using Decoder for bytes;

mapping(address => bool) public isOperator;

event OrderFill(
address operator,
address maker,
bytes32 orderHash,
uint256 offeredAmount
);
event OperatorAccessModified(address indexed authorized, bool access);

constructor(
string memory name,
string memory version
) EIP712(name, version) {}

modifier onlyOperator() {
if (!isOperator[msg.sender]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

revert NotAnOperator(msg.sender);
}
_;
}

function manageOperatorPrivilege(
address _address,
bool _access
) external onlyOwner {
if (_address == address(0)) {
revert ZeroAddress();
}

// TBD: should we not allow if owner is trying to set same access? (con: additional gas)
// Overwrites the access previously granted.
isOperator[_address] = _access;

emit OperatorAccessModified(_address, _access);
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xhammadghazi
The natspec for signatures seems missing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

acknowledged

*
* @param orders An array of order structs representing the orders to be filled.
* @param clearingPrices An array of clearing prices that the facilitator is offering to the makers.
* @param facilitatorInteractionCalldata The calldata for the facilitator's interaction.
* @param facilitatorInteractionTargetContract The address of the facilitator's target contract.
* @param offeredAmounts An array of clearing prices that the facilitator is offering to the makers.
* @param facilitatorInteraction The calldata for the facilitator's interaction.
*/
function fillOrders(
OrderEngine.Order[] calldata orders,
uint256[] calldata clearingPrices,
bytes calldata facilitatorInteractionCalldata,
address facilitatorInteractionTargetContract
) external {
uint256[] calldata offeredAmounts,
bytes[] calldata signatures,
bytes calldata facilitatorInteraction
) external onlyOperator {
// TBD: Private orders?

// TBD: max array length check needed? Considering fn will be restricted to operators only

/**
TBD: no need to check for clearingPrices length to be equal to 0 as if that's the case, txn will revert in subsequent check
but should we check for clearingPrices length to be equal to zero explicitly for better error reporting?
TBD: no need to check for offeredAmounts length to be equal to 0 as if that's the case, txn will revert in subsequent check
but should we check for offeredAmounts length to be equal to zero explicitly for better error reporting?
Also consider generic error message
*/
// Revert if the orders array length is zero.
Expand All @@ -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) {
Copy link
Contributor

@gul-hameed gul-hameed Oct 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes indeed

revert ArraysLengthMismatch(orders.length, offeredAmounts.length);
}

for (uint256 i; i < orders.length; ) {
OrderEngine.Order calldata order = orders[i];
bytes calldata signature = signatures[i];

bytes32 orderHash = order.hash();
bytes32 orderMessageHash = _hashTypedDataV4(orderHash);

if (block.timestamp > order.validTill) {
revert OrderExpired(orderHash);
}

if (order.buyTokenAmount == 0 || order.sellTokenAmount == 0) {
revert ZeroTokenAmounts();
}

if (
order.maker == address(0) ||
order.buyToken == address(0) ||
order.sellToken == address(0) ||
order.recipient == address(0)
) {
revert ZeroAddress();
}

// STUB: PARTIAL FILL FEATURE //

// TBD: debatable, can take signing scheme in order schema or can verify like 1inch
if (order.isContract()) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I will take it into consideration.

if (
!(IERC1271(order.maker).isValidSignature(
orderMessageHash,
signature
) == IERC1271.isValidSignature.selector)
) {
revert BadSignature();
}
} else {
address signer = ECDSA.recover(orderMessageHash, signature);
if (signer != order.maker) {
revert BadSignature();
}
}

// STUB: VERIFY PREDICATES //

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (order.preInteraction.length >= 20) {
// proceed only if interaction length is enough to store address
(
address interactionTarget,
bytes calldata interactionData
) = order.preInteraction.decodeTargetAndCalldata();
IPreInteractionNotificationReceiver(interactionTarget)
.fillOrderPreInteraction(
orderMessageHash,
order.maker,
offeredAmounts[i],
interactionData
);
}

// TODO: reorder params type
_receiveAsset(order.sellToken, order.sellTokenAmount, order.maker);

unchecked {
++i;
}
}

// Revert if the facilitator has provided calldata for its interaction but has provided null target contract address.
if (
facilitatorInteractionCalldata.length != 0 &&
facilitatorInteractionTargetContract == address(0)
) {
revert ZeroFacilitatorTargetAddress(); // TBD: use generic error message i.e. ZeroAddress()
if (facilitatorInteraction.length >= 20) {
// proceed only if interaction length is enough to store address
(
address interactionTarget,
bytes calldata interactionData
) = facilitatorInteraction.decodeTargetAndCalldata();
IInteractionNotificationReceiver(interactionTarget)
.fillOrderInteraction(
msg.sender,
orders,
offeredAmounts,
interactionData
);
}

// Loop start
// Perform order specific sanity checks
// Verify signatjures
// Verify predicates
// Call pre-interaction hook
// Transfer funds from maker to vault
// Loop end
// Call facilitator interaction
// Loop start
// Ensure facilitator is respecting maker price
// Transfer funds from vault to maker
// Call post-interaction hook
// Emit event (decide where to emit event, as its considered as an effect so maybe do it somewhere in the start)
// TODO: Need optimization
for (uint256 i; i < orders.length; ) {
OrderEngine.Order calldata order = orders[i];

bytes32 orderHash = order.hash();
bytes32 orderMessageHash = _hashTypedDataV4(orderHash);

if (order.buyTokenAmount > offeredAmounts[i]) {
revert LimitPriceNotRespected(
order.buyTokenAmount,
offeredAmounts[i]
);
}

// TODO: reorder params type
_sendAsset(order.buyToken, offeredAmounts[i], order.recipient);

if (order.postInteraction.length >= 20) {
// proceed only if interaction length is enough to store address
(
address interactionTarget,
bytes calldata interactionData
) = order.postInteraction.decodeTargetAndCalldata();
IPostInteractionNotificationReceiver(interactionTarget)
.fillOrderPostInteraction(
orderMessageHash,
order.maker,
offeredAmounts[i],
interactionData
);
}

// TODO: decide where to emit event, as its considered as an effect so maybe do it somewhere in the start; what params to log;
// events spam? ; consider emitting just one event
emit OrderFill(
msg.sender,
order.maker,
orderMessageHash,
offeredAmounts[i]
);

unchecked {
++i;
}
}
}
}
7 changes: 7 additions & 0 deletions src/AdvancedOrderEngineErrors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,10 @@ pragma solidity 0.8.21;
error ArraysLengthMismatch(uint256 ordersArrLen, uint256 clearingPricesArrLen);
error ZeroFacilitatorTargetAddress();
error EmptyOrdersArray();
error OrderExpired(bytes32 orderHash);
error ZeroTokenAmounts();
error ZeroAddress();
error BadSignature();
error IncorrectDataLength();
error LimitPriceNotRespected(uint256 desiredAmount, uint256 offeredAmount);
error NotAnOperator(address caller);
28 changes: 28 additions & 0 deletions src/Vault.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
// SPDX-License-Identifier: MIT
pragma solidity 0.8.21;

import {SafeERC20} from "openzeppelin/token/ERC20/utils/SafeERC20.sol";
import {IERC20} from "openzeppelin/token/ERC20/IERC20.sol";

abstract contract Vault {
using SafeERC20 for IERC20;

function _receiveAsset(
address asset,
uint256 amount,
address maker
) internal {
IERC20 token = _asIERC20(asset);

token.safeTransferFrom(maker, address(this), amount);
}

function _sendAsset(address asset, uint256 amount, address maker) internal {
IERC20 token = _asIERC20(asset);
token.safeTransfer(maker, amount);
}

function _asIERC20(address asset) internal pure returns (IERC20) {
return IERC20(asset);
}
}
24 changes: 24 additions & 0 deletions src/interfaces/IInteractionNotificationReceiver.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
// SPDX-License-Identifier: MIT

pragma solidity 0.8.21;

import {OrderEngine} from "../libraries/OrderEngine.sol";

/**
* @notice Interface for facilitator interaction hook, it is invoked after funds are transferred from the 'maker' to the 'vault'.
*/
interface IInteractionNotificationReceiver {
/**
* @notice Callback method that gets called after funds transfer from the 'maker' to the 'vault'.
* @param operator Address of the caller who executed orders on behalf of the facilitator.
* @param orders Orders the facilitator is willing to fill.
* @param offeredAmounts Amounts of the asset the facilitator is offering to the makers.
* @param interactionData Interaction calldata
*/
function fillOrderInteraction(
address operator,
OrderEngine.Order[] calldata orders,
uint256[] calldata offeredAmounts,
bytes memory interactionData
) external;
}
22 changes: 22 additions & 0 deletions src/interfaces/IPostInteractionNotificationReceiver.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
// SPDX-License-Identifier: MIT

pragma solidity 0.8.21;

/**
* @notice Interface for maker post-interaction hook, it is invoked after funds are transferred from the 'vault' to the 'maker'.
*/
interface IPostInteractionNotificationReceiver {
/**
* @notice Callback method that gets called after all funds transfers
* @param orderHash Hash of the order being processed
* @param maker Maker address
* @param clearingPrice Actual amount maker will receive
* @param interactionData Interaction calldata
*/
function fillOrderPostInteraction(
bytes32 orderHash,
address maker,
uint256 clearingPrice,
bytes memory interactionData
) external;
}
Loading
Loading