From 1bcae19face3eb04af372469881eb1ae1962fae0 Mon Sep 17 00:00:00 2001 From: Federico Giacon <58218759+fedgiac@users.noreply.github.com> Date: Thu, 1 Sep 2022 17:48:03 +0000 Subject: [PATCH] Add reentrancy guard (#11) --- src/CoWSwapEthFlow.sol | 16 +++++++- src/vendored/ReentrancyGuard.sol | 66 ++++++++++++++++++++++++++++++++ test/CallOnReceive.sol | 50 ++++++++++++++++++++++++ test/CoWSwapEthFlow.t.sol | 41 ++++++++++++++++++++ 4 files changed, 171 insertions(+), 2 deletions(-) create mode 100644 src/vendored/ReentrancyGuard.sol create mode 100644 test/CallOnReceive.sol diff --git a/src/CoWSwapEthFlow.sol b/src/CoWSwapEthFlow.sol index 322deb8..6857b80 100644 --- a/src/CoWSwapEthFlow.sol +++ b/src/CoWSwapEthFlow.sol @@ -6,11 +6,13 @@ import "./interfaces/ICoWSwapSettlement.sol"; import "./interfaces/ICoWSwapEthFlow.sol"; import "./mixins/CoWSwapOnchainOrders.sol"; import "./vendored/GPv2EIP1271.sol"; +import "./vendored/ReentrancyGuard.sol"; /// @title CoW Swap ETH Flow /// @author CoW Swap Developers contract CoWSwapEthFlow is CoWSwapOnchainOrders, + ReentrancyGuard, EIP1271Verifier, ICoWSwapEthFlow { @@ -47,6 +49,7 @@ contract CoWSwapEthFlow is function createOrder(EthFlowOrder.Data calldata order) external payable + nonReentrant returns (bytes32 orderHash) { if (msg.value != order.sellAmount) { @@ -84,7 +87,10 @@ contract CoWSwapEthFlow is } /// @inheritdoc ICoWSwapEthFlow - function deleteOrder(EthFlowOrder.Data calldata order) external { + function deleteOrder(EthFlowOrder.Data calldata order) + external + nonReentrant + { GPv2Order.Data memory cowSwapOrder = order.toCoWSwapOrder( wrappedNativeToken ); @@ -113,7 +119,13 @@ contract CoWSwapEthFlow is uint256 freedAmount = cowSwapOrder.sellAmount - cowSwapSettlement.filledAmount(orderUid); - if (!payable(orderData.owner).send(freedAmount)) { + // Using low level calls to perform the transfer avoids setting arbitrary limits to the amount of gas used in a + // call. Reentrancy is avoided thanks to the `nonReentrant` function modifier. + // solhint-disable-next-line avoid-low-level-calls + (bool success, ) = payable(orderData.owner).call{value: freedAmount}( + "" + ); + if (!success) { revert EthTransferFailed(); } } diff --git a/src/vendored/ReentrancyGuard.sol b/src/vendored/ReentrancyGuard.sol new file mode 100644 index 0000000..87fbb70 --- /dev/null +++ b/src/vendored/ReentrancyGuard.sol @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts v4.4.1 (security/ReentrancyGuard.sol) + +// Vendored from OpenZeppelin Contracts v4.7.3, see: +// + +pragma solidity ^0.8.0; + +/** + * @dev Contract module that helps prevent reentrant calls to a function. + * + * Inheriting from `ReentrancyGuard` will make the {nonReentrant} modifier + * available, which can be applied to functions to make sure there are no nested + * (reentrant) calls to them. + * + * Note that because there is a single `nonReentrant` guard, functions marked as + * `nonReentrant` may not call one another. This can be worked around by making + * those functions `private`, and then adding `external` `nonReentrant` entry + * points to them. + * + * TIP: If you would like to learn more about reentrancy and alternative ways + * to protect against it, check out our blog post + * https://blog.openzeppelin.com/reentrancy-after-istanbul/[Reentrancy After Istanbul]. + */ +abstract contract ReentrancyGuard { + // Booleans are more expensive than uint256 or any type that takes up a full + // word because each write operation emits an extra SLOAD to first read the + // slot's contents, replace the bits taken up by the boolean, and then write + // back. This is the compiler's defense against contract upgrades and + // pointer aliasing, and it cannot be disabled. + + // The values being non-zero value makes deployment a bit more expensive, + // but in exchange the refund on every call to nonReentrant will be lower in + // amount. Since refunds are capped to a percentage of the total + // transaction's gas, it is best to keep them low in cases like this one, to + // increase the likelihood of the full refund coming into effect. + uint256 private constant _NOT_ENTERED = 1; + uint256 private constant _ENTERED = 2; + + uint256 private _status; + + constructor() { + _status = _NOT_ENTERED; + } + + /** + * @dev Prevents a contract from calling itself, directly or indirectly. + * Calling a `nonReentrant` function from another `nonReentrant` + * function is not supported. It is possible to prevent this from happening + * by making the `nonReentrant` function external, and making it call a + * `private` function that does the actual work. + */ + modifier nonReentrant() { + // On the first call to nonReentrant, _notEntered will be true + require(_status != _ENTERED, "ReentrancyGuard: reentrant call"); + + // Any calls to nonReentrant after this point will fail + _status = _ENTERED; + + _; + + // By storing the original value once again, a refund is triggered (see + // https://eips.ethereum.org/EIPS/eip-2200) + _status = _NOT_ENTERED; + } +} diff --git a/test/CallOnReceive.sol b/test/CallOnReceive.sol new file mode 100644 index 0000000..8b0a573 --- /dev/null +++ b/test/CallOnReceive.sol @@ -0,0 +1,50 @@ +// SPDX-License-Identifier: LGPL-3.0-or-later +pragma solidity ^0.8; + +// Used to test reentrancy when receiving ETH +contract CallOnReceive { + address payable public to; + uint256 public value; + bytes public data; + + // Store call result for later retrieval + bool public lastFallbackCallSuccess; + bytes public lastFallbackCallReturnData; + + receive() external payable { + // solhint-disable-next-line avoid-low-level-calls + (lastFallbackCallSuccess, lastFallbackCallReturnData) = to.call{ + value: value + }(data); + } + + function execCall( + address payable _to, + uint256 _value, + bytes memory _data + ) public returns (bytes memory) { + // solhint-disable-next-line avoid-low-level-calls + (bool success, bytes memory result) = _to.call{value: _value}(_data); + if (success == false) { + // Forward revert error + // solhint-disable-next-line no-inline-assembly + assembly { + let ptr := mload(0x40) + let size := returndatasize() + returndatacopy(ptr, 0, size) + revert(ptr, size) + } + } + return result; + } + + function setCallOnReceive( + address payable _to, + uint256 _value, + bytes calldata _data + ) external { + to = _to; + value = _value; + data = _data; + } +} diff --git a/test/CoWSwapEthFlow.t.sol b/test/CoWSwapEthFlow.t.sol index 6a7674a..98723b6 100644 --- a/test/CoWSwapEthFlow.t.sol +++ b/test/CoWSwapEthFlow.t.sol @@ -8,6 +8,7 @@ import "forge-std/Test.sol"; import "./Constants.sol"; import "./CoWSwapEthFlow/CoWSwapEthFlowExposed.sol"; import "./FillWithSameByte.sol"; +import "./CallOnReceive.sol"; import "./Reverter.sol"; import "../src/interfaces/ICoWSwapOnchainOrders.sol"; import "../src/vendored/GPv2EIP1271.sol"; @@ -421,6 +422,46 @@ contract OrderDeletion is EthFlowTestSetup { vm.stopPrank(); } + + function testNonReentrant() public { + CallOnReceive owner = new CallOnReceive(); + EthFlowOrder.Data memory ethFlowOrder1 = dummyOrder(); + EthFlowOrder.Data memory ethFlowOrder2 = dummyOrder(); + ethFlowOrder1.appData ^= bytes32(type(uint256).max); // Change any parameter to get a different order. + OrderDetails memory orderDetails1 = orderDetails(ethFlowOrder1); + + // Create first order + vm.deal(address(owner), ethFlowOrder1.sellAmount); + owner.execCall( + payable(address(ethFlow)), + ethFlowOrder1.sellAmount, + abi.encodeCall(ethFlow.createOrder, ethFlowOrder1) + ); + + // Prepare creating second order on reentrancy. CallOnReceive is configured to execute that call. It does *not* + // revert if the call reverts but store the result of the call in storage. + vm.deal(address(owner), ethFlowOrder2.sellAmount); + owner.setCallOnReceive( + payable(address(ethFlow)), + ethFlowOrder2.sellAmount, + abi.encodeCall(ethFlow.createOrder, ethFlowOrder2) + ); + + // Delete order with attempted reentrancy. + mockOrderFilledAmount(orderDetails1.orderUid, 0); + owner.execCall( + payable(address(ethFlow)), + 0, + abi.encodeCall(ethFlow.deleteOrder, ethFlowOrder1) + ); + + assertFalse(owner.lastFallbackCallSuccess()); + bytes memory reentrancyRevertData = abi.encodeWithSignature( + "Error(string)", // https://docs.soliditylang.org/en/v0.8.16/control-structures.html?highlight=%22Error(string)%22#panic-via-assert-and-error-via-require + "ReentrancyGuard: reentrant call" + ); + assertEq(owner.lastFallbackCallReturnData(), reentrancyRevertData); + } } contract SignatureVerification is EthFlowTestSetup {