Smart Contract Security Vulnerabilities

A Case Study with Real Solidity Code

Introduction

Anticipating that the smart contracts we create can potentially garner massive funding for our users, our security auditing process ensures that our contracts are not susceptible to common attacks at the very least. This article is an addendum to How to Decentralize the Film Industry to explain the security measures we took in our Solidity code.

For the design patterns we followed based on industry best practices, take a look at this related article:

Smart Contract Design Patterns

Contract Source Code on Remix:

https://gist.github.com/thonly/dba764a0bc28544f479d81ba174e1511

BoxOffice.sol is our main contract, a factory that creates instances of BoxOfficeMovie.sol per film project. BoxOfficeMovie inherits the ERC20 standard contract from OpenZeppelin and uses its SafeMath library to securely perform arithmetic operations. The HeartBankCoin contract and the Oracle contract and the OracleLibrary and OracleStorage contracts it depends are ancillary while OracleRegistry is for demonstrating the Upgradable Registry pattern.

Integer Overflow and Underflow

Attackers can take advantage of the fact that integers can overflow and underflow in the EVM. To protect against this, we import the SafeMath library from OpenZeppelin to use in our arithmetic calculations, e.g.:

using SafeMath for uint;
using SafeMath for uint8;
modifier chargeWithdrawFee(uint amount) {
    uint fee = withdrawFee.mul(amount).div(100);
    heartbank = heartbank.add(fee);
    charity = charity.add(fee);
    _;
}

Forcibly Sending Ether to a Contract

Attackers can also take advantage of the subtlety that ether can be sent to a contract without ever triggering its callback function. This can be achieved by self-destructing a malicious contract and specifying the target contract as the recipient of the remaining ether. Attackers can also precompute a contract’s address ahead of time and send ether to it before the contract is even deployed! For these reasons, we design our contracts to not depend on the contract balance for logic. Instead, we use storage variables to keep track of the admin’s balance from collected fees and the filmmaker’s balance from ticket sales, irrespective of how much ether our contracts have received:

uint public heartbank; // admin's balance from collected fees
    
uint public fund; // filmmaker's balance from ticket sales

Timestamp Dependence

The now keyword is an alias for block.timestamp, which miners can manipulate to within a 30-second window. In our dApp, filmmakers can specify a salesEndDate to which buyers must adhere. Though our buyTickets() function depends on now to perform this check, it’s not a sensitive requirement as 30-second manipulation by miners can be tolerated.

function buyTickets(address movie, uint quantity) 
    public 
    payable
    stopInEmergency
    returns (bool)
{
    require(quantity > 0);
    Movie film = Movie(movie);
    uint price = film.price();
        
    // only during sales period
    require(now < film.salesEndDate());
        
    // check available tickets
    require(quantity <= film.availableTickets());
        
    // check payment amount
    require(msg.value >= quantity.mul(price));
        
    // check excess payment
    uint excess = msg.value.sub(quantity.mul(price));
    if (excess > 0) emit ExcessPaid(now, movie, msg.sender, excess);
        
    film.buyTickets(msg.sender, quantity);
    emit TicketsBought(movie, msg.sender, price, quantity);
    return true;
 }

Front Running

Closely related to timestamp dependence is Transaction-Ordering Dependence, aka Front Running. When transactions are broadcasted to the network, they first enter the mempool from which miners can choose which ones and in which order they want to include in their block (within a 30-second window). This can be problematic because they know in advance what transactions are about to occur. Fortunately for us, this is not problematic because our sales period requirement is not a critical check.

// only during sales period
require(now < film.salesEndDate());

Reentrancy

Whenever <address>.transfer() is invoked, it provides an opportunity for control to flow to a corrupt contract, because address needs not always be an externally owned account. When called, the called contract’s fallback function can contain code to call back (reenter) the calling contract’s function in a recursive manner that can be detrimental. For our withdrawFund() function to work, we cannot avoid making an external call to the recipient’s address. Therefore, we utilize the withdrawal design pattern to separate the accounting logic from the transfer logic, making sure to finish all internal state changes first before invoking the external call. Usually, this means transfer() is the last step. Moreover, potential cross-function race conditions such as calling internal functions that call external functions are also designed away.

function withdrawFund(address movie, address recipient, uint amount, string expense) 
    public 
    stopInEmergency
    onlyFilmmaker(movie)
    chargeWithdrawFee(amount)
    returns (bool)
{
    require(recipient != address(0));
    require(amount > 0);
    require(bytes(expense).length > 0);
        Movie(movie).withdrawFund(amount.add(withdrawFee.mul(amount).div(100)));
        
    emit FundWithdrawn(now, movie, recipient, amount, expense);
    recipient.transfer(amount);
    return true;
}

Finishing all internal work before making external calls also protects against denial of service attacks. The called contract’s fallback can always revert, which also halts the execution of the calling contract. If there are necessary operations further along, they will never get executed.

By utilizing the withdrawal design pattern to favor pull payments over push payments, we also avoid denial of service attacks that target the block gas limit. We could have looped through an array of addresses, pushing payments to the recipients automatically for their convenience. However, attackers could take advantage of the fact that we are looping through an array of unknown size, creating many corrupt recipient accounts to cause our function to hit the block gas limit whenever it executes. The effect, our contract becomes unusable.

Did we miss anything? Let us know!

How to Decentralize the Film Industry

Reference: https://consensys.github.io/smart-contract-best-practices/known_attacks