26236 - [SC - Insight] Malicious DeGate Operator EOA can irreversibly ...

Submitted on Nov 29th 2023 at 00:53:43 UTC by @ongrid for Boost | DeGate

Report ID: #26236

Report type: Smart Contract

Report severity: Insight

Target: https://etherscan.io/address/0x9C07A72177c5A05410cA338823e790876E79D73B#code

Impacts:

  • Irreversible stop of Exchange

  • Prevent new users from registering (Zero Knowledge Proof Circuit)

  • Permanent freezing of unclaimed rewards

  • Griefing (e.g. no profit motive for an attacker, but damage to the users or the protocol)

  • Contract fails to deliver promised returns, but doesn't lose value

  • Force DeGate into Exodus Mode

  • The account cannot be used (Zero Knowledge Proof Circuit)

  • Temporary freezing of funds: Minimum freezing of 15 days (Zero Knowledge Proof Circuit)

Description

Bug Description

The current ExchangeProxy at 0x9C07A72177c5A05410cA338823e790876E79D73B is managed by LoopringIOExchangeOwner 0x9b93e47b7F61ad1358Bd47Cd01206708E85AE5eD, which is in turn controlled by an Externally Owned Account (EOA) 0xacD3A62F3eED1BfE4fF0eC8240d645c1F5477F82`. This setup poses significant security risks due to centralized control, allowing the key holder to unilaterally modify critical contract parameters and potentially shut down the system irreversibly.

Impact

Irreversible System Shutdown

The DeGate Operator, using an EOA, can entirely shut down the system. This can be executed by invoking Exchange.shutdown() through LoopringIOExchangeOwner.transact(abiEncodedCall), as demonstrated in the testExchangeShutdown test case.

Unilateral Modification of Critical Platform Configuration Parameters

In the testSetMaxAgeDepositUntilWithdrawable and testExchangeSetDepositParams tests, it's shown how the operator can change key Exchange settings via the same call chain initiated from their EOA.

Risk Breakdown

Difficulty to Exploit: Easy

Recommendation

To mitigate centralized control risks, Exchange's ownership (not just the proxy facade but also the business logic) should be restructured:

  • Transfer Ownership to Multisig: Change the ownership of LoopringIOExchangeOwner to a Gnosis Multisig wallet, distributing control among multiple parties.

  • Consider Using Timelock: Use Timelock contract as the owner to introduce a delay in critical operations, enhancing transparency.

Proof of concept

I have created a forge test, functioning on an Ethereum mainnet fork using the addreses of the live contracts. The file demonstrating vulnerabilities is MaliciousOperatorStopsExchangeContract.t.sol

Code (git)

commit 625637a9e64fa298b3a80b3216382c3382656f97
Author: Kirill Varlamov <kirill@ongrid.pro>
Date:   Tue Nov 28 03:12:40 2023 +0400

    test: malicious DeGate operator stops Exchange (centralized control by EOA)

diff --git a/foundry.toml b/foundry.toml
index 25b918f9..aa662d8a 100644
--- a/foundry.toml
+++ b/foundry.toml
@@ -3,4 +3,5 @@ src = "src"
 out = "out"
 libs = ["lib"]
 
+solc = '0.8.23'
 # See more config options https://github.com/foundry-rs/foundry/blob/master/crates/config/README.md#all-options
diff --git a/src/Counter.sol b/src/Counter.sol
deleted file mode 100644
index aded7997..00000000
--- a/src/Counter.sol
+++ /dev/null
@@ -1,14 +0,0 @@
-// SPDX-License-Identifier: UNLICENSED
-pragma solidity ^0.8.13;
-
-contract Counter {
-    uint256 public number;
-
-    function setNumber(uint256 newNumber) public {
-        number = newNumber;
-    }
-
-    function increment() public {
-        number++;
-    }
-}
diff --git a/test/Counter.t.sol b/test/Counter.t.sol
deleted file mode 100644
index e9b9e6ac..00000000
--- a/test/Counter.t.sol
+++ /dev/null
@@ -1,24 +0,0 @@
-// SPDX-License-Identifier: UNLICENSED
-pragma solidity ^0.8.13;
-
-import {Test, console2} from "forge-std/Test.sol";
-import {Counter} from "../src/Counter.sol";
-
-contract CounterTest is Test {
-    Counter public counter;
-
-    function setUp() public {
-        counter = new Counter();
-        counter.setNumber(0);
-    }
-
-    function test_Increment() public {
-        counter.increment();
-        assertEq(counter.number(), 1);
-    }
-
-    function testFuzz_SetNumber(uint256 x) public {
-        counter.setNumber(x);
-        assertEq(counter.number(), x);
-    }
-}
diff --git a/test/MaliciousOperatorStopsExchangeContract.t.sol b/test/MaliciousOperatorStopsExchangeContract.t.sol
new file mode 100644
index 00000000..52685b34
--- /dev/null
+++ b/test/MaliciousOperatorStopsExchangeContract.t.sol
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.10;
+
+import "forge-std/Test.sol";
+import "test/mock/IExchangeV3.sol";
+import "test/mock/ILoopringIOExchangeOwner.sol";
+
+contract ShutdownExchangeTest is Test {
+    event DepositParamsUpdate(
+        uint256 freeDepositMax, uint256 freeDepositRemained, uint256 freeSlotPerBlock, uint256 depositFee
+    );
+
+    IExchangeV3 exchange;
+    address exchangeOwner;
+    ILoopringIOExchangeOwner ioex;
+    address ioexOwner;
+
+    function setUp() public {
+        uint256 fork;
+        fork = vm.createFork("https://mainnet.infura.io/v3/bcb7b03333384d49957cc9b4b53daa1d");
+        vm.selectFork(fork);
+        exchange = IExchangeV3(0x9C07A72177c5A05410cA338823e790876E79D73B);
+        ioex = ILoopringIOExchangeOwner(0x9b93e47b7F61ad1358Bd47Cd01206708E85AE5eD);
+        ioexOwner = ioex.owner();
+    }
+
+    function testLoopringIOExchangeOwnerIsEOA() public {
+        // ioexOwner is EOA (i.e. centralized control)
+        assertFalse(isContr