Skip to content

Commit

Permalink
Add reentrancy guard (#11)
Browse files Browse the repository at this point in the history
  • Loading branch information
fedgiac authored Sep 1, 2022
1 parent aa79574 commit 1bcae19
Show file tree
Hide file tree
Showing 4 changed files with 171 additions and 2 deletions.
16 changes: 14 additions & 2 deletions src/CoWSwapEthFlow.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -47,6 +49,7 @@ contract CoWSwapEthFlow is
function createOrder(EthFlowOrder.Data calldata order)
external
payable
nonReentrant
returns (bytes32 orderHash)
{
if (msg.value != order.sellAmount) {
Expand Down Expand Up @@ -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
);
Expand Down Expand Up @@ -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();
}
}
Expand Down
66 changes: 66 additions & 0 deletions src/vendored/ReentrancyGuard.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// SPDX-License-Identifier: MIT
// OpenZeppelin Contracts v4.4.1 (security/ReentrancyGuard.sol)

// Vendored from OpenZeppelin Contracts v4.7.3, see:
// <https://raw.githubusercontent.com/OpenZeppelin/openzeppelin-contracts/v4.7.3/contracts/security/ReentrancyGuard.sol>

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;
}
}
50 changes: 50 additions & 0 deletions test/CallOnReceive.sol
Original file line number Diff line number Diff line change
@@ -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;
}
}
41 changes: 41 additions & 0 deletions test/CoWSwapEthFlow.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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 {
Expand Down

0 comments on commit 1bcae19

Please sign in to comment.