Factories Improve Smart Contract Security

Smart contracts can deploy other smart contracts. This enables the factory pattern, where instead of one smart contract that keeps track of many things, you create many smart contracts that each keep track of just one thing. Using this pattern can simplify your code and reduce the impact of certain kinds of security vulnerabilities.

In this post, I’ll walk you through an example that’s loosely based on a critical vulnerability we discovered during a recent audit. If the factory pattern had been used, the vulnerability would have been much less severe.

A Buggy Smart Contract

Below is a smart contract that sells WETH through a rather simple interface. If you have WETH, you just need to approve this smart contract to sell your tokens, and it will ensure your get paid the correct amount. Anyone can buy WETH from anyone as long as enough tokens have been approved. (Presumably, sellers advertise the sale somewhere off chain.)

The contract uses the withdrawal pattern to deliver the payment to the sellers, but the contract’s author has made a serious mistake:

 1// Technically this could sell any token, but we're selling WETH in this
 2// example because then I don't have to think about prices. 1 WETH costs 1 ETH.
 3contract WETHMarket {
 4    IERC20 public weth;
 5    mapping(address => uint256) public balanceOf;
 6
 7    constructor(IERC20 _weth) public {
 8        weth = _weth;
 9    }
10
11    // Buy WETH from a specified seller. Seller must first approve WETH.
12    function buyFrom(address seller) external payable {
13        balanceOf[seller] += msg.value;
14        require(weth.transferFrom(seller, msg.sender, msg.value),
15            "WETH transfer failed.");
16    }
17
18    // Used by a seller to get their ETH.
19    function withdraw(uint256 amount) external {
20        require(amount <= balanceOf[msg.sender], "Insufficient funds.");
21
22        // Whoops! Forgot this:
23        // balanceOf[msg.sender] -= amount;
24
25        (bool success, ) = msg.sender.call.value(amount)("");
26        require(success, "ETH transfer failed.");
27    }
28}

(If you’re wondering why the code uses .call instead of .transfer, go read “Stop Using Solidity’s Transfer() Now”).

Because the seller’s balance is never reduced, a seller who is owed any ether at all can just call withdraw() repeatedly to drain the contract of everyone’s ether. This is a critical vulnerability.

Fixing this bug, like most bugs, is trivial once you’ve spotted it. But in this post, I’d like to talk about how the bug could have been mitigated by using the factory pattern, even if we had no knowledge of this particular issue.

A Contract With Less Responsibilty

Let’s now look at a simpler version of the WETHMarket contract. In this version, the contract is responsible for selling only a single seller’s WETH. This contract has the same bug as the previous version:

 1contract WETHSale {
 2    IERC20 public weth;
 3    address seller; // only a single seller
 4    uint256 public balance; // no need for a mapping anymore
 5
 6    constructor(IERC20 _weth, address _seller) public {
 7        weth = _weth;
 8        seller = _seller;
 9    }
10
11    // No need to specify the seller.
12    function buy() external payable {
13        balance += msg.value;
14        require(weth.transferFrom(seller, msg.sender, msg.value));
15    }
16
17    function withdraw(uint256 amount) external {
18        require(msg.sender == seller, "Only the seller can withdraw.");
19        require(amount <= balance, "Insufficient funds.");
20
21        uint256 amount = balance;
22
23        // Whoops! Forgot this:
24        // balance -= amount;
25
26        (bool success, ) = msg.sender.call.value(amount)("");
27        require(success, "ETH transfer failed.");
28    }
29}

Despite being the exact same logical error, this vulnerability is not nearly as severe. Only a single account is allowed to call withdraw(), and all the ether stored in the contract belongs to that account anyway. The impact of this bug is just that balance doesn’t reflect the true balance in the contract.

This bug was hand-picked to show the benefit, but this bug is representative of a large class of bugs around escrow. In my experience auditing smart contracts, this is one of the most common places to find critical vulnerabilities.

The idea behind escrow is that different funds must be kept separate to ensure the contract can always cover everything that’s owed. One of the easiest ways to get escrow right is to separate funds into different smart contracts altogether.

You can think of the factory pattern as a defense-in-depth approach to escrow.

Simpler Code

Not only does the single-seller version of the contract have a more robust escrow, but it’s also just simpler. We got rid of a function parameter and a mapping. In production code, we would probably go one step farther and remove balance altogether in favor of address(this).balance.

Because I wrote the contract specifically to be easy to read, the original code was already very simple. In real-world examples, the difference can be much more dramatic. Any opportunity to reduce complexity is a win from a security perspective.

The Factory

Each seller could just deploy their own WETHSale contract and get the benefits of the simpler contract, but this approach has a major drawback. A malicious seller could deploy a slightly modified version of the code that didn’t actually transfer WETH.

Even if a reputable firm like ConsenSys Diligence audited the WETHSale code, each buyer would have to verify that the specific contract they were buying from used that exact code.

Using a factory solves this problem. The factory ensures that each deployed contract uses identical code, and it provides an easy lookup mechanism to find the singular contract for a given seller:

contract WETHSaleFactory {
    IERC20 public weth;
    mapping(address => WETHSale) public sales;

    constructor(IERC20 _weth) public {
        weth = _weth;
    }

    function deploy() external {
        require(sales[msg.sender] == WETHSale(0), "Only one sale per seller.");

        sales[msg.sender] = new WETHSale(weth, msg.sender);
    }
}

Potential Drawbacks

As a security auditor, I often encourage clients to use factories, but the decision is more nuanced.

A major drawback to using the factory pattern is that it’s expensive. The CREATE op code currently has a gas cost of 32,000. Our particular contract also has to do two extra SSTOREs to keep track of the WETH and seller addresses, each costing 20,000 gas. That’s at least 72,000 gas more than the original multiple-seller version of the code.

Another potential drawback is complexity. In most real-world cases, the factory pattern simplifies your existing contracts, but keep in mind that it also adds a new one: the factory itself. Depending on your code, this may have a net effect of increasing complexity.

Before making a decision about the factory pattern, carefully consider the overall impact of the change.

Summary

• Mistakes around escrow are a significant cause of critical vulnerabilities.
• Using separate smart contracts reduces the severity of those bugs.
• The factory pattern enables this in a trustless way.
• Also consider the potential drawbacks before adopting the factory pattern.

All posts