From b51eb3a91e78229a4667a045650578c642e7875d Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Wed, 31 Aug 2022 10:23:29 +0000 Subject: [PATCH] Add order deletion (#7) * Add order deletion * Add parameter test to dedicated test contract * Remove time check from test * Fix usage of vm.startPrank and vm.prank * Use .send to transfer ETH --- src/CoWSwapEthFlow.sol | 52 +++- src/interfaces/ICoWSwapEthFlow.sol | 12 + src/interfaces/ICoWSwapSettlement.sol | 16 ++ src/libraries/EthFlowOrder.sol | 4 + test/CoWSwapEthFlow.t.sol | 264 +++++++++++++++++- test/CoWSwapEthFlow/CoWSwapEthFlowExposed.sol | 3 +- test/Reverter.sol | 11 + 7 files changed, 344 insertions(+), 18 deletions(-) create mode 100644 src/interfaces/ICoWSwapSettlement.sol create mode 100644 test/Reverter.sol diff --git a/src/CoWSwapEthFlow.sol b/src/CoWSwapEthFlow.sol index e512acd..2463664 100644 --- a/src/CoWSwapEthFlow.sol +++ b/src/CoWSwapEthFlow.sol @@ -2,6 +2,7 @@ pragma solidity ^0.8; import "./libraries/EthFlowOrder.sol"; +import "./interfaces/ICoWSwapSettlement.sol"; import "./interfaces/ICoWSwapEthFlow.sol"; import "./mixins/CoWSwapOnchainOrders.sol"; @@ -9,6 +10,12 @@ import "./mixins/CoWSwapOnchainOrders.sol"; /// @author CoW Swap Developers contract CoWSwapEthFlow is CoWSwapOnchainOrders, ICoWSwapEthFlow { using EthFlowOrder for EthFlowOrder.Data; + using GPv2Order for GPv2Order.Data; + using GPv2Order for bytes; + + /// @dev The address of the CoW Swap settlement contract that will be used to settle orders created by this + /// contract. + ICoWSwapSettlement public immutable cowSwapSettlement; /// @dev The address of the contract representing the default native token in the current chain (e.g., WETH for /// Ethereum mainnet). @@ -21,11 +28,13 @@ contract CoWSwapEthFlow is CoWSwapOnchainOrders, ICoWSwapEthFlow { /// onchain data. mapping(bytes32 => EthFlowOrder.OnchainData) public orders; - /// @param _cowSwapSettlementContract The address of the CoW Swap settlement contract. + /// @param _cowSwapSettlement The address of the CoW Swap settlement contract. /// @param _wrappedNativeToken The address of the default native token in the current chain (e.g., WETH on mainnet). - constructor(address _cowSwapSettlementContract, IERC20 _wrappedNativeToken) - CoWSwapOnchainOrders(_cowSwapSettlementContract) - { + constructor( + ICoWSwapSettlement _cowSwapSettlement, + IERC20 _wrappedNativeToken + ) CoWSwapOnchainOrders(address(_cowSwapSettlement)) { + cowSwapSettlement = _cowSwapSettlement; wrappedNativeToken = _wrappedNativeToken; } @@ -68,4 +77,39 @@ contract CoWSwapEthFlow is CoWSwapOnchainOrders, ICoWSwapEthFlow { orders[orderHash] = onchainData; } + + /// @inheritdoc ICoWSwapEthFlow + function deleteOrder(EthFlowOrder.Data calldata order) external { + GPv2Order.Data memory cowSwapOrder = order.toCoWSwapOrder( + wrappedNativeToken + ); + bytes32 orderHash = cowSwapOrder.hash(cowSwapDomainSeparator); + + EthFlowOrder.OnchainData memory orderData = orders[orderHash]; + + if ( + orderData.owner == EthFlowOrder.INVALIDATED_OWNER || + orderData.owner == EthFlowOrder.NO_OWNER || + (// solhint-disable-next-line not-rely-on-time + orderData.validTo >= block.timestamp && + orderData.owner != msg.sender) + ) { + revert NotAllowedToDeleteOrder(orderHash); + } + + orders[orderHash].owner = EthFlowOrder.INVALIDATED_OWNER; + + bytes memory orderUid = new bytes(GPv2Order.UID_LENGTH); + orderUid.packOrderUidParams( + orderHash, + address(this), + cowSwapOrder.validTo + ); + uint256 freedAmount = cowSwapOrder.sellAmount - + cowSwapSettlement.filledAmount(orderUid); + + if (!payable(orderData.owner).send(freedAmount)) { + revert EthTransferFailed(); + } + } } diff --git a/src/interfaces/ICoWSwapEthFlow.sol b/src/interfaces/ICoWSwapEthFlow.sol index 7c88eb5..e4b8ad9 100644 --- a/src/interfaces/ICoWSwapEthFlow.sol +++ b/src/interfaces/ICoWSwapEthFlow.sol @@ -13,6 +13,12 @@ interface ICoWSwapEthFlow { /// @dev Error thrown when trying to create an order without sending the expected amount of ETH to this contract. error IncorrectEthAmount(); + /// @dev Error thrown if trying to delete an order while not allowed. + error NotAllowedToDeleteOrder(bytes32 orderHash); + + /// @dev Error thrown when unsuccessfully sending ETH to an address. + error EthTransferFailed(); + /// @dev Function that creates and broadcasts an ETH flow order that sells native ETH. The order is paid for when /// the caller sends out the transaction. The caller takes ownership of the new order. /// @@ -23,4 +29,10 @@ interface ICoWSwapEthFlow { external payable returns (bytes32 orderHash); + + /// @dev Marks an existing ETH flow order as invalid and refunds the trader of all ETH that hasn't been traded yet. + /// Note that some parameters of the order are ignored, as for example the order expiration date and the quote id. + /// + /// @param order The order to be deleted. + function deleteOrder(EthFlowOrder.Data calldata order) external; } diff --git a/src/interfaces/ICoWSwapSettlement.sol b/src/interfaces/ICoWSwapSettlement.sol new file mode 100644 index 0000000..5c75be2 --- /dev/null +++ b/src/interfaces/ICoWSwapSettlement.sol @@ -0,0 +1,16 @@ +// SPDX-License-Identifier: LGPL-3.0-or-later +pragma solidity ^0.8; + +/// @title CoW Swap Settlement Contract Interface +/// @author CoW Swap Developers +/// @dev This interface collects the functions of the CoW Swap settlement contract that are used by the ETH flow +/// contract. +interface ICoWSwapSettlement { + /// @dev Map each user order by UID to the amount that has been filled so + /// far. If this amount is larger than or equal to the amount traded in the + /// order (amount sold for sell orders, amount bought for buy orders) then + /// the order cannot be traded anymore. If the order is fill or kill, then + /// this value is only used to determine whether the order has already been + /// executed. + function filledAmount(bytes memory orderUid) external returns (uint256); +} diff --git a/src/libraries/EthFlowOrder.sol b/src/libraries/EthFlowOrder.sol index ec9ad64..bb6fb53 100644 --- a/src/libraries/EthFlowOrder.sol +++ b/src/libraries/EthFlowOrder.sol @@ -47,6 +47,10 @@ library EthFlowOrder { /// @dev An order that is owned by this address is an order that has not yet been assigned. address public constant NO_OWNER = address(0); + /// @dev An order that is owned by this address is an order that has been invalidated. Note that this address cannot + /// be directly used to create orders. + address public constant INVALIDATED_OWNER = address(type(uint160).max); + /// @dev Error returned if the receiver of the ETH flow order is unspecified (`GPv2Order.RECEIVER_SAME_AS_OWNER`). error ReceiverMustBeSet(); diff --git a/test/CoWSwapEthFlow.t.sol b/test/CoWSwapEthFlow.t.sol index 760389b..3415f67 100644 --- a/test/CoWSwapEthFlow.t.sol +++ b/test/CoWSwapEthFlow.t.sol @@ -1,25 +1,53 @@ // SPDX-License-Identifier: LGPL-3.0-or-later pragma solidity ^0.8; +// solhint-disable reason-string +// solhint-disable not-rely-on-time + import "forge-std/Test.sol"; import "./Constants.sol"; import "./CoWSwapEthFlow/CoWSwapEthFlowExposed.sol"; import "./FillWithSameByte.sol"; +import "./Reverter.sol"; import "../src/interfaces/ICoWSwapOnchainOrders.sol"; -contract TestCoWSwapEthFlow is Test, ICoWSwapOnchainOrders { - using EthFlowOrder for EthFlowOrder.Data; - using GPv2Order for GPv2Order.Data; - +contract EthFlowTestSetup is Test { CoWSwapEthFlowExposed internal ethFlow; IERC20 internal wrappedNativeToken = IERC20(0x1234567890123456789012345678901234567890); - address internal cowSwap = Constants.COWSWAP_ADDRESS; + ICoWSwapSettlement internal cowSwap = + ICoWSwapSettlement(Constants.COWSWAP_ADDRESS); function setUp() public { ethFlow = new CoWSwapEthFlowExposed(cowSwap, wrappedNativeToken); } + // Unfortunately, even if the order mapping takes a bytes32 and returns a struct, Solidity interprets the output + // struct as a tuple instead. This wrapping function puts back the ouput into the same struct. + function ordersMapping(bytes32 orderHash) + internal + view + returns (EthFlowOrder.OnchainData memory) + { + (address owner, uint32 validTo) = ethFlow.orders(orderHash); + return EthFlowOrder.OnchainData(owner, validTo); + } + + function mockOrderFilledAmount(bytes memory orderUid, uint256 amount) + public + { + vm.mockCall( + address(cowSwap), + abi.encodeWithSelector( + ICoWSwapSettlement.filledAmount.selector, + orderUid + ), + abi.encode(amount) + ); + } +} + +contract TestDeployment is EthFlowTestSetup { function testDeploymentParams() public { assertEq( address(ethFlow.wrappedNativeToken()), @@ -30,6 +58,11 @@ contract TestCoWSwapEthFlow is Test, ICoWSwapOnchainOrders { Constants.COWSWAP_TEST_DOMAIN_SEPARATOR ); } +} + +contract TestOrderCreation is EthFlowTestSetup, ICoWSwapOnchainOrders { + using EthFlowOrder for EthFlowOrder.Data; + using GPv2Order for GPv2Order.Data; function testRevertOrderCreationIfNotEnoughEthSent() public { uint256 sellAmount = 1 ether; @@ -74,19 +107,17 @@ contract TestCoWSwapEthFlow is Test, ICoWSwapOnchainOrders { vm.deal(executor1, sellAmount); vm.deal(executor2, sellAmount); - vm.startPrank(executor1); + vm.prank(executor1); ethFlow.createOrder{value: sellAmount}(order); - vm.stopPrank(); - vm.startPrank(executor2); vm.expectRevert( abi.encodeWithSelector( ICoWSwapEthFlow.OrderIsAlreadyOwned.selector, orderHash ) ); + vm.prank(executor2); ethFlow.createOrder{value: sellAmount}(order); - vm.stopPrank(); } function testOrderCreationReturnsOrderHash() public { @@ -137,7 +168,6 @@ contract TestCoWSwapEthFlow is Test, ICoWSwapOnchainOrders { address executor = address(0x1337); vm.deal(executor, sellAmount); - vm.startPrank(executor); vm.expectEmit(true, true, true, true, address(ethFlow)); emit ICoWSwapOnchainOrders.OrderPlacement( executor, @@ -145,8 +175,8 @@ contract TestCoWSwapEthFlow is Test, ICoWSwapOnchainOrders { signature, abi.encodePacked(quoteId, validTo) ); + vm.prank(executor); ethFlow.createOrder{value: sellAmount}(order); - vm.stopPrank(); } function testOrderCreationSetsExpectedOnchainOrderInformation() public { @@ -172,9 +202,8 @@ contract TestCoWSwapEthFlow is Test, ICoWSwapOnchainOrders { address executor = address(0x1337); vm.deal(executor, sellAmount); - vm.startPrank(executor); + vm.prank(executor); ethFlow.createOrder{value: sellAmount}(order); - vm.stopPrank(); (address ethFlowOwner, uint32 ethFlowValidTo) = ethFlow.orders( orderHash @@ -183,3 +212,212 @@ contract TestCoWSwapEthFlow is Test, ICoWSwapOnchainOrders { assertEq(ethFlowValidTo, validTo); } } + +contract OrderDeletion is EthFlowTestSetup { + using EthFlowOrder for EthFlowOrder.Data; + using GPv2Order for GPv2Order.Data; + using GPv2Order for bytes; + + struct OrderDetails { + EthFlowOrder.Data data; + bytes32 hash; + bytes orderUid; + } + + function orderDetails(EthFlowOrder.Data memory order) + internal + view + returns (OrderDetails memory) + { + bytes32 orderHash = order.toCoWSwapOrder(wrappedNativeToken).hash( + ethFlow.cowSwapDomainSeparatorPublic() + ); + bytes memory orderUid = new bytes(GPv2Order.UID_LENGTH); + orderUid.packOrderUidParams( + orderHash, + address(ethFlow), + type(uint32).max + ); + return OrderDetails(order, orderHash, orderUid); + } + + function dummyOrder() internal view returns (EthFlowOrder.Data memory) { + EthFlowOrder.Data memory order = EthFlowOrder.Data( + IERC20(FillWithSameByte.toAddress(0x01)), + FillWithSameByte.toAddress(0x02), + FillWithSameByte.toUint256(0x03), + FillWithSameByte.toUint256(0x04), + FillWithSameByte.toBytes32(0x05), + FillWithSameByte.toUint256(0x06), + FillWithSameByte.toUint32(0x07), + true, + FillWithSameByte.toUint64(0x08) + ); + require( + order.validTo > block.timestamp, + "Dummy order is already expired, please update dummy expiration value" + ); + return order; + } + + function createOrderWithOwner(OrderDetails memory order, address owner) + public + { + vm.deal(owner, order.data.sellAmount); + vm.prank(owner); + ethFlow.createOrder{value: order.data.sellAmount}(order.data); + } + + function testCanDeleteValidOrdersIfOwner() public { + address owner = address(0x424242); + EthFlowOrder.Data memory ethFlowOrder = dummyOrder(); + OrderDetails memory order = orderDetails(ethFlowOrder); + createOrderWithOwner(order, owner); + mockOrderFilledAmount(order.orderUid, 0); + + vm.prank(owner); + ethFlow.deleteOrder(order.data); + } + + function testCanDeleteExpiredOrdersIfNotOwner() public { + address owner = address(0x424242); + address executor = address(0x1337); + EthFlowOrder.Data memory ethFlowOrder = dummyOrder(); + ethFlowOrder.validTo = uint32(block.timestamp) - 1; + OrderDetails memory order = orderDetails(ethFlowOrder); + createOrderWithOwner(order, owner); + mockOrderFilledAmount(order.orderUid, 0); + + vm.prank(executor); + ethFlow.deleteOrder(order.data); + } + + function testCannotDeleteValidOrdersIfNotOwner() public { + address owner = address(0x424242); + address executor = address(0x1337); + EthFlowOrder.Data memory ethFlowOrder = dummyOrder(); + ethFlowOrder.validTo = uint32(block.timestamp) + 1; + OrderDetails memory order = orderDetails(ethFlowOrder); + createOrderWithOwner(order, owner); + mockOrderFilledAmount(order.orderUid, 0); + + vm.expectRevert( + abi.encodeWithSelector( + ICoWSwapEthFlow.NotAllowedToDeleteOrder.selector, + order.hash + ) + ); + vm.prank(executor); + ethFlow.deleteOrder(order.data); + } + + function testOrderDeletionSetsOrderAsInvalidated() public { + address owner = address(0x424242); + OrderDetails memory order = orderDetails(dummyOrder()); + createOrderWithOwner(order, owner); + mockOrderFilledAmount(order.orderUid, 0); + + assertEq(ordersMapping(order.hash).owner, owner); + vm.prank(owner); + ethFlow.deleteOrder(order.data); + assertEq( + ordersMapping(order.hash).owner, + EthFlowOrder.INVALIDATED_OWNER + ); + } + + function testOrderDeletionSendsEthBack() public { + // Using owner != executor to make certain that the ETH were not sent to msg.sender + address owner = address(0x424242); + address executor = address(0x1337); + EthFlowOrder.Data memory ethFlowOrder = dummyOrder(); + ethFlowOrder.validTo = uint32(block.timestamp) - 1; + OrderDetails memory order = orderDetails(ethFlowOrder); + createOrderWithOwner(order, owner); + mockOrderFilledAmount(order.orderUid, 0); + + assertEq(owner.balance, 0); + vm.prank(executor); + ethFlow.deleteOrder(order.data); + assertEq(owner.balance, order.data.sellAmount); + } + + function testOrderDeletionRevertsIfDeletingUninitializedOrder() public { + OrderDetails memory order = orderDetails(dummyOrder()); + + vm.expectRevert( + abi.encodeWithSelector( + ICoWSwapEthFlow.NotAllowedToDeleteOrder.selector, + order.hash + ) + ); + ethFlow.deleteOrder(order.data); + } + + function testOrderDeletionRevertsIfDeletingOrderTwice() public { + address owner = address(0x424242); + OrderDetails memory order = orderDetails(dummyOrder()); + createOrderWithOwner(order, owner); + mockOrderFilledAmount(order.orderUid, 0); + + vm.startPrank(owner); + + ethFlow.deleteOrder(order.data); + vm.expectRevert( + abi.encodeWithSelector( + ICoWSwapEthFlow.NotAllowedToDeleteOrder.selector, + order.hash + ) + ); + ethFlow.deleteOrder(order.data); + + vm.stopPrank(); + } + + function testOrderDeletionForPartiallyFilledOrders() public { + address owner = address(0x424242); + OrderDetails memory order = orderDetails(dummyOrder()); + createOrderWithOwner(order, owner); + uint256 filledAmount = 1337; + mockOrderFilledAmount(order.orderUid, filledAmount); + + assertEq(owner.balance, 0); + vm.prank(owner); + ethFlow.deleteOrder(order.data); + assertEq(owner.balance, order.data.sellAmount - filledAmount); + } + + function testOrderDeletionRevertsIfSendingEthFails() public { + address owner = address(new Reverter()); + OrderDetails memory order = orderDetails(dummyOrder()); + createOrderWithOwner(order, owner); + mockOrderFilledAmount(order.orderUid, 0); + + vm.expectRevert(ICoWSwapEthFlow.EthTransferFailed.selector); + vm.prank(owner); + ethFlow.deleteOrder(order.data); + } + + function testCannotCreateSameOrderOnceDeleted() public { + address owner = address(0x424242); + EthFlowOrder.Data memory ethFlowOrder = dummyOrder(); + ethFlowOrder.validTo = uint32(block.timestamp) - 1; + OrderDetails memory order = orderDetails(ethFlowOrder); + mockOrderFilledAmount(order.orderUid, 0); + + vm.deal(owner, order.data.sellAmount); + vm.startPrank(owner); + + ethFlow.createOrder{value: order.data.sellAmount}(order.data); + ethFlow.deleteOrder(order.data); + vm.expectRevert( + abi.encodeWithSelector( + ICoWSwapEthFlow.OrderIsAlreadyOwned.selector, + order.hash + ) + ); + ethFlow.createOrder{value: order.data.sellAmount}(order.data); + + vm.stopPrank(); + } +} diff --git a/test/CoWSwapEthFlow/CoWSwapEthFlowExposed.sol b/test/CoWSwapEthFlow/CoWSwapEthFlowExposed.sol index 670c4cb..e4540aa 100644 --- a/test/CoWSwapEthFlow/CoWSwapEthFlowExposed.sol +++ b/test/CoWSwapEthFlow/CoWSwapEthFlowExposed.sol @@ -2,10 +2,11 @@ pragma solidity ^0.8; import "../../src/CoWSwapEthFlow.sol"; +import "../../src/interfaces/ICoWSwapSettlement.sol"; /// @dev Wrapper that exposes internal funcions of CoWSwapEthFlow. contract CoWSwapEthFlowExposed is CoWSwapEthFlow { - constructor(address settlementContractAddress, IERC20 weth) + constructor(ICoWSwapSettlement settlementContractAddress, IERC20 weth) CoWSwapEthFlow(settlementContractAddress, weth) // solhint-disable-next-line no-empty-blocks { diff --git a/test/Reverter.sol b/test/Reverter.sol new file mode 100644 index 0000000..7c4f31b --- /dev/null +++ b/test/Reverter.sol @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: LGPL-3.0-or-later +pragma solidity ^0.8; + +// It's not possible to mock reverting a function. +// https://github.com/foundry-rs/foundry/issues/2740 +// This contract is a workaround to trigger a revert. +contract Reverter { + receive() external payable { + revert("Mock revert"); + } +}