diff --git a/.github/workflows/slither.yml b/.github/workflows/slither.yml new file mode 100644 index 0000000..1d2a8c2 --- /dev/null +++ b/.github/workflows/slither.yml @@ -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/slither-action@v0.3.0 + 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 "actions@github.com" + + - 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 diff --git a/foundry.toml b/foundry.toml index ad7e0d0..30f53b6 100644 --- a/foundry.toml +++ b/foundry.toml @@ -6,6 +6,7 @@ libs = ['lib'] auto_detect_solc = false solc = '0.8.21' +via_ir = true optimizer = true optimizer_runs = 10_000 diff --git a/slither-output.txt b/slither-output.txt new file mode 100644 index 0000000..8b13789 --- /dev/null +++ b/slither-output.txt @@ -0,0 +1 @@ + diff --git a/src/AdvancedOrderEngine.sol b/src/AdvancedOrderEngine.sol index 33dcf3e..91e121e 100644 --- a/src/AdvancedOrderEngine.sol +++ b/src/AdvancedOrderEngine.sol @@ -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]) { + 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. * * @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. @@ -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) { + 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()) { + 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 // + + 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; + } + } } } diff --git a/src/AdvancedOrderEngineErrors.sol b/src/AdvancedOrderEngineErrors.sol index 8270062..50f1a02 100644 --- a/src/AdvancedOrderEngineErrors.sol +++ b/src/AdvancedOrderEngineErrors.sol @@ -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); diff --git a/src/Vault.sol b/src/Vault.sol new file mode 100644 index 0000000..a0cc65e --- /dev/null +++ b/src/Vault.sol @@ -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); + } +} diff --git a/src/interfaces/IInteractionNotificationReceiver.sol b/src/interfaces/IInteractionNotificationReceiver.sol new file mode 100644 index 0000000..b9b3ce0 --- /dev/null +++ b/src/interfaces/IInteractionNotificationReceiver.sol @@ -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; +} diff --git a/src/interfaces/IPostInteractionNotificationReceiver.sol b/src/interfaces/IPostInteractionNotificationReceiver.sol new file mode 100644 index 0000000..137e159 --- /dev/null +++ b/src/interfaces/IPostInteractionNotificationReceiver.sol @@ -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; +} diff --git a/src/interfaces/IPreInteractionNotificationReceiver.sol b/src/interfaces/IPreInteractionNotificationReceiver.sol new file mode 100644 index 0000000..41f3b61 --- /dev/null +++ b/src/interfaces/IPreInteractionNotificationReceiver.sol @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: MIT + +pragma solidity 0.8.21; + +/** + * @notice Interface for maker pre-interaction hook, it is invoked before funds are transferred from the 'maker' to the 'vault'. + */ +interface IPreInteractionNotificationReceiver { + /** + * @notice Callback method that gets called before any 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 fillOrderPreInteraction( + bytes32 orderHash, + address maker, + uint256 clearingPrice, + bytes memory interactionData + ) external; +} diff --git a/src/libraries/Decoder.sol b/src/libraries/Decoder.sol new file mode 100644 index 0000000..7feb06f --- /dev/null +++ b/src/libraries/Decoder.sol @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: MIT +pragma solidity 0.8.21; + +import "../AdvancedOrderEngineErrors.sol"; + +library Decoder { + function decodeTargetAndCalldata( + bytes calldata data + ) internal pure returns (address target, bytes calldata args) { + if (data.length < 20) revert IncorrectDataLength(); + // no memory ops inside so this insertion is automatically memory safe + assembly { + // solhint-disable-line no-inline-assembly + target := shr(96, calldataload(data.offset)) + args.offset := add(data.offset, 20) + args.length := sub(data.length, 20) + } + } +} diff --git a/src/libraries/OrderEngine.sol b/src/libraries/OrderEngine.sol index 124e541..a5b9beb 100644 --- a/src/libraries/OrderEngine.sol +++ b/src/libraries/OrderEngine.sol @@ -12,7 +12,7 @@ library OrderEngine { uint256 feeAmounts; address maker; address taker; // null on public orders - address recipient; // TODO: maybe use null to represent msg.sender? + address recipient; // TBD: use null to represent maker? Right now expecting explicit address set address sellToken; address buyToken; bool isPartiallyFillable; @@ -20,6 +20,55 @@ library OrderEngine { bytes predicates; bytes preInteraction; bytes postInteraction; - bytes signature; + } + + bytes32 public constant ORDER_TYPE_HASH = + keccak256( + "Order(" + "uint256 nonce," + "uint256 validTill," + "uint256 sellTokenAmount," + "uint256 buyTokenAmount," + "uint256 feeAmounts," + "address maker," + "address taker," + "address recipient," + "address sellToken," + "address buyToken," + "bool isPartiallyFillable," + "bytes32 extraData," + "bytes predicates," + "bytes preInteraction," + "bytes postInteraction" + ")" + ); + + function hash(Order calldata order) public pure returns (bytes32) { + return ( + keccak256( + abi.encode( + ORDER_TYPE_HASH, + order.nonce, + order.validTill, + order.sellTokenAmount, + order.buyTokenAmount, + order.feeAmounts, + order.maker, + order.taker, + order.recipient, + order.sellToken, + order.buyToken, + order.isPartiallyFillable, + order.extraData, + keccak256(order.predicates), + keccak256(order.preInteraction), + keccak256(order.postInteraction) + ) + ) + ); + } + + function isContract(Order calldata order) public view returns (bool) { + return order.maker.code.length > 0; } }