chore(Adapter): add ReentrancyGuard to settle()

pnetwork-association · Aug 15, 2024 · e909c69 · e909c69
1 parent 9953a68
commit e909c69
Show file tree

Hide file tree

Showing 3 changed files with 74 additions and 4 deletions.
diff --git a/solidity/src/Adapter.sol b/solidity/src/Adapter.sol
@@ -2,8 +2,9 @@
 pragma solidity ^0.8.25;
 
 import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
-import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
+import {ReentrancyGuard} from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
 
 import {IPAM} from "./interfaces/IPAM.sol";
 import {IPAM} from "./interfaces/IPAM.sol";
@@ -16,7 +17,7 @@ import {IXERC20Registry} from "./interfaces/IXERC20Registry.sol";
 import {IXERC20Lockbox} from "./interfaces/IXERC20Lockbox.sol";
 import {ExcessivelySafeCall} from "./libraries/ExcessivelySafeCall.sol";
 
-contract Adapter is IAdapter, Ownable {
+contract Adapter is IAdapter, Ownable, ReentrancyGuard {
     using ExcessivelySafeCall for address;
 
     bytes32 public constant SWAP_EVENT_TOPIC =
@@ -52,11 +53,10 @@ contract Adapter is IAdapter, Ownable {
     }
 
     /// @inheritdoc IAdapter
-    // TODO: check reentrancy here
     function settle(
         Operation memory operation,
         IPAM.Metadata calldata metadata
-    ) external {
+    ) external nonReentrant {
         if (operation.erc20 != bytes32(abi.encode(erc20)))
             revert InvalidOperation();
 

diff --git a/solidity/src/test/DataReceiverReentrancy.sol b/solidity/src/test/DataReceiverReentrancy.sol
@@ -0,0 +1,21 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+import {Adapter} from "../Adapter.sol";
+import {IPAM} from "../interfaces/IPAM.sol";
+import {IAdapter} from "../interfaces/IAdapter.sol";
+import {IPReceiver} from "../interfaces/IPReceiver.sol";
+
+contract DataReceiverReentrancy is IPReceiver {
+    event DataReceived(bytes userdata);
+    function receiveUserData(bytes calldata userdata) external {
+        (
+            IAdapter.Operation memory operation,
+            IPAM.Metadata memory metadata
+        ) = abi.decode(userdata, (IAdapter.Operation, IPAM.Metadata));
+
+        Adapter(msg.sender).settle(operation, metadata);
+
+        emit DataReceived(userdata);
+    }
+}
diff --git a/solidity/test/forge/Integration.t.sol b/solidity/test/forge/Integration.t.sol
@@ -18,6 +18,7 @@ import {XERC20} from "../../src/xerc20/XERC20.sol";
 import {ERC20Test} from "../../src/test/ERC20Test.sol";
 import {DataReceiver} from "../../src/test/DataReceiver.sol";
 import {XERC20Lockbox} from "../../src/xerc20/XERC20Lockbox.sol";
+import {DataReceiverReentrancy} from "../../src/test/DataReceiverReentrancy.sol";
 
 import "forge-std/console.sol";
 
@@ -373,8 +374,56 @@ contract IntegrationTest is Test, Helper {
 
         uint256 R = xerc20_B.balanceOf(address(receiver));
         uint256 A = xerc20_B.balanceOf(address(adapter_B));
+        uint256 T = xerc20_B.totalSupply();
 
         assertEq(R, amount - fees);
         assertEq(A, 0);
+        assertEq(T, amount - fees);
+    }
+
+    function test_settle_e2e_RevertWhen_ReentrancyAttack() public {
+        uint256 amount = 10000;
+
+        DataReceiverReentrancy receiver = new DataReceiverReentrancy();
+
+        // This is operation and signed metadata
+        // crafted for the test
+        bytes memory data = vm.parseBytes(
+            "0x000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000001a0ad7c5bef027816a800da1736444fb58a807ef4c9603b7848673f7e3a68eb14a52a80e1ef1d7842f27f2e6be0972bb708b9a135c38860dbe73c27c3486c34f4de00000000000000000000000000000000000000000000000000000000000000000000000000000000000000002946259e0334f33a064106302415ad3391bed3840000000000000000000000000000000000000000000000000000000000007a690000000000000000000000000000000000000000000000000000000000007a6a00000000000000000000000000000000000000000000000000000000000026fc0000000000000000000000002b5ad5c4795c026514f8317c7a215e218dccd6cf0000000000000000000000006813eb9362372eef6200f3b1dbc3f819671cba690000000000000000000000000000000000000000000000000000000000000140000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000280000000000000000000000000000000000000000000000000000000000000020201010000000000000000000000000000000000000000000000000000000000007a69ad7c5bef027816a800da1736444fb58a807ef4c9603b7848673f7e3a68eb14a52a80e1ef1d7842f27f2e6be0972bb708b9a135c38860dbe73c27c3486c34f4de0000000000000000000000006d411e0a54382ed43f02410ce1c7a7c122afa6e1a68959eed8a7e77ce926c4c04ee06434559ae1db7f636ceacd659f5c9126f1c30000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000ea00000000000000000000000000000000000000000000000000000000000000000000000000000000000000002946259e0334f33a064106302415ad3391bed3840000000000000000000000000000000000000000000000000000000000007a6a00000000000000000000000000000000000000000000000000000000000026fc0000000000000000000000002b5ad5c4795c026514f8317c7a215e218dccd6cf000000000000000000000000000000000000000000000000000000000000002a3078363831334562393336323337324545463632303066336231646243336638313936373163424136390000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000416493671d8cea48c1f89a2b0599493a1f6309b69df5214e737dbcb1440cd5c41061b1977499b273c161f4ac2fc7a4d4f0e8d0ce5a052b384019d218220fdc78a81c00000000000000000000000000000000000000000000000000000000000000"
+        );
+
+        vm.recordLogs();
+        _performERC20Swap(
+            CHAIN_A,
+            address(erc20_A),
+            user,
+            address(adapter_A),
+            CHAIN_B,
+            address(receiver), // recipient
+            amount,
+            data
+        );
+
+        Vm.Log[] memory logs = vm.getRecordedLogs();
+        operation = _getOperationFromLogs(logs, SWAP_TOPIC);
+        metadata = _getMetadataFromLogs(
+            logs,
+            SWAP_TOPIC,
+            operation,
+            attestatorPrivateKey
+        );
+
+        bytes32 eventId = _getEventId(metadata.preimage);
+
+        vm.chainId(CHAIN_B);
+
+        // Traces shows the failure is due to ReentrancyGuardReentrantCall()
+        // but we can't test it due to https://book.getfoundry.sh/cheatcodes/expect-revert#description
+        vm.expectEmit(address(adapter_B));
+        emit IAdapter.ReceiveUserDataFailed();
+        vm.expectEmit(address(adapter_B));
+        emit IAdapter.Settled(eventId);
+
+        adapter_B.settle(operation, metadata);
     }
 }