Smart Contract Vulnerabilities

A collection of smart contract vulnerabilities along with prevention methods

Access Control

Math

Control Flow

Data Handling

Unsafe Logic

Code Quality

Authorization Through tx.origin

tx.origin is a global variable in Solidity which returns the address that sent a transaction. It's important that you never use tx.origin for authorization since another contract can use a fallback function to call your contract and gain authorization since the authorized address is stored in tx.origin. Consider this example:

pragma solidity >=0.5.0 <0.7.0;

// THIS CONTRACT CONTAINS A BUG - DO NOT USE
contract TxUserWallet {
    address owner;

    constructor() public {
        owner = msg.sender;
    }

    function transferTo(address payable dest, uint amount) public {
        require(tx.origin == owner);
        dest.transfer(amount);
    }
}

Here we can see that the TxUserWallet contract authorizes the transferTo() function with tx.origin.

pragma solidity >=0.5.0 <0.7.0;

interface TxUserWallet {
    function transferTo(address payable dest, uint amount) external;
}

contract TxAttackWallet {
    address payable owner;

    constructor() public {
        owner = msg.sender;
    }

    function() external {
        TxUserWallet(msg.sender).transferTo(owner, msg.sender.balance);
    }
}

Now if someone were to trick you into sending ether to the TxAttackWallet contract address, they can steal your funds by checking tx.origin to find the address that sent the transaction.

To prevent this kind of attack, use msg.sender for authorization.

Examples from: https://solidity.readthedocs.io/en/develop/security-considerations.html#tx-origin

Sources

Insufficient Access Control

Access control is often imperative in management and ownership of smart contracts. It's important to consider ways in which access control may be circumvented, or insufficiently implemented and the corresponding consequences. Contracts may allow any user to execute sensitive functions without verifying their authorization status. For example, a contract may have functions that transfer ownership, mint tokens, or modify critical state variables without checking if the caller has the appropriate permissions.

Improper implementation of access control can lead to severe vulnerabilities, allowing unauthorized users to manipulate the contract’s state or even drain its funds. For example:

// UNSECURE
function setInterestRate(uint256 _interestRate) public {
    // No access modifier, so anyone can change the interestRate
    interestRate = _interestRate;
}

The setInterestRate function doesn’t have any access control which means anyone can call it to change the interestRate which can lead to severe consequences. We can resolve this by adding authorization logic, e.g.:

modifier onlyOwner() {
    require(msg.sender == owner, "Only the owner can call this function");
    _;
}

function setInterestRate(uint256 _interestRate) public onlyOwner {
    interestRate = _interestRate;
}

Now, the onlyOwner modifier has been added. This modifier checks if the caller of a function is the contract owner before allowing the function to proceed, ensuring that only the owner of the contract can change the interestRate. Similarly, role based access control mechanisms and whitelisting mechanisms can be implemented for proper access control.

Sources

Delegatecall to Untrusted Callee

Delegatecall is a special variant of a message call. It is almost identical to a regular message call except the target address is executed in the context of the calling contract and msg.sender and msg.value remain the same. Essentially, delegatecall delegates other contracts to modify the calling contract's storage.

Since delegatecall gives so much control over a contract, it's very important to only use this with trusted contracts such as your own. If the target address comes from user input, be sure to verify that it is a trusted contract.

Example

Consider the following contracts where delegatecall is misused, leading to a vulnerability:

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.16;

contract Proxy {
    address public owner;

    constructor() {
        owner = msg.sender;
    }

    function forward(address callee, bytes calldata _data) public {
        require(callee.delegatecall(_data), "Delegatecall failed");
    }
}

contract Target {
    address public owner;

    function pwn() public {
        owner = msg.sender;
    }
}

contract Attack {
    address public proxy;

    constructor(address _proxy) {
        proxy = _proxy;
    }

    function attack(address target) public {
        Proxy(proxy).forward(target, abi.encodeWithSignature("pwn()"));
    }
}

In this example, the Proxy contract uses delegatecall to forward any call it receives to an address provided by the user. The Target contract contains a call to the pwn() function that changes the owner of the contract to the caller.

The Attack contract takes advantage of this setup by calling the forward function of the Proxy contract, passing the address of the Target contract and the encoded function call pwn(). This results in the Proxy contract's storage being modified, specifically the owner variable, which is set to the attacker’s address.

Mitigations

To mitigate the risks associated with delegatecall to untrusted callees, consider the following strategies:

  1. Whitelist Trusted Contracts: Ensure that the target address for delegatecall is a contract you control or a contract that is part of a verified and trusted list.

  2. Limit the Scope of Delegatecall: Use delegatecall only for specific, controlled operations. Avoid exposing it as a general-purpose function unless absolutely necessary.

Sources

Signature Malleability

It's generally assumed that a valid signature cannot be modified without the private key and remain valid. However, it is possible to modify and signature and maintain validity. One example of a system which is vulnerable to signature malleability is one in which validation as to whether an action can be executed is determined based on whether the signature has been previously used.

// UNSECURE
require(!signatureUsed[signature]);

// Validate signer and perform state modifying logic
...

signatureUsed[signature] = true;

In the above example, we can see that the signature is saved in a signatureUsed mapping after execution and validated to not exist in that mapping before execution. The problem with this is that if the signature can be modified while maintaining validity, the transaction can be repeated by an attacker.

How it works

To understand how signature malleability works, we first need to understand a bit about elliptic curve cryptography.

An elliptic curve consists of all the points that satisfy an equation of the form:

$y^2 = x^3 + ax + b$

where

$4a^3 + 27b^2 \not= 0$ (to avoid singular points)

Some examples:

Elliptic Curves

Note that the curves are always symmetrical about the x-axis

The curve used by Ethereum is secp256k1, which looks like this:

secp256k1

Now that we understand the basics of elliptic curve cryptography, we can dig into how signature malleability actually works on Ethereum.

Ethereum uses ECDSA as its signature scheme. ECDSA signatures consist of a pair of numbers, $(r, s)$, with an integer order $n$. As a result of the x-axis symmetry, if $(r, s)$ is a valid signature, then so is $(r, -s$ mod $n)$.

It's possible to calculate this complementary signature without knowing the private key used to produce it in the first place, which gives an attacker the ability to produce a second valid signature.

Mitigation

To avoid this issue, it's imperative to recognize that validating that a signature is not reused is insufficient in enforcing that the transaction is not replayed.

Sources

Missing Protection against Signature Replay Attacks

Sometimes in smart contracts it is necessary to perform signature verification to improve usability and gas cost. However, consideration needs to be taken when implementing signature verification. To protect against Signature Replay Attacks, the contract should only be allowing new hashes to be processed. This prevents malicious users from replaying another users signature multiple times.

To be extra safe with signature verification, follow these recommendations:

  • Store every message hash processed by the contract, then check messages hashes against the existing ones before executing the function.
  • Include the address of the contract in the hash to ensure that the message is only used in a single contract.
  • Never generate the message hash including the signature. See Signature Malleability

Sources

Integer Overflow and Underflow

In solidity, Integer types have maximum and minimum values. Integer overflow occurs when an integer variable exceeds the maximum value that can be stored in that variable type. Similarly, Integer underflow occurs when an integer variable goes below the minimum value for that variable type. Example: The maximum value uint8 can store is 255. Now, when you store 256 in uint8 it will overflow and the value will reset to 0. When you store 257, the value will be 1, 2 for 258 and so on. Similarly, if you try to store -1 in the uint8 variable the value of the variable will become 255, and so on as it will underflow.

Some integer types and their min/max values:

TypeMaxMin
uint82550
uint16655350
uint24167772150
uint2562^256 - 10

Since smaller integer types like: uint8, uint16, etc have smaller maximum values, it can be easier to cause an overflow, thus they should be used with greater caution.

To prevent over/underflows, Safe Math Library is often used by contracts with older versions of Solidity but Solidity >=0.8 protects against integer overflows and underflows through the use of built-in safe math functions. It's important to consider that regardless of SafeMath logic being used, either built-in or used manually in older contracts, over/underflows still trigger reverts, which may result in denial of service of important functionality or other unexpected effects. Even after the update of solidity to 0.8, there are scenarios in which the integer overflow and underflow can still occur without the transaction reverting.

Typecasting

The most common way in which integer over/underflow is possible when you convert an integer of a larger uint data type to a smaller data type.

uint256 public a = 258;
uint8 public b = uint8(a); // typecasting uint256 to uint8

The above code snippet will overflow and the 2 will be stored in the variable b due to the fact that maximum value in uint8 data type is 255. So, it will overflow and reset to 0 without reverting.

Using Shift Operators

Overflow & underflow checks are not performed for shift operations like they are performed for other arithmetic operations. Thus, over/underflows can occur.

The left shift << operator shifts all the beats in the first operand by the number specified in the second operand. Shifting an operand by 1 position is equivalent to multiplying it by 2, shifting 2 positions is equivalent to multiplying it by 4, and shifting 3 positions is equivalent to multiplying by 8.

uint8 public a = 100;
uint8 public b = 2;

uint8 public c = a << b; // overflow as 100 * 4 > 255

In the above code, left shifting a which is 100 by 2 positions b is equivalent to multiplying 100 by 4. So the result will overflow and the value in c will be 144 because 400-256 is 144.

Use of Inline Assembly:

Inline Assembly in solidity is performed using YUL language. In YUL programming language, integer underflow & overflow is possible as compiler does not check automatically for it as YUL is a low-level language that is mostly used for making the code more optimized, which does this by omitting many opcodes.

uint8 public a = 255;

function addition() public returns (uint8 result) {
    assembly {
        result := add(sload(a.slot), 1) // adding 1 will overflow and reset to 0
        // using inline assembly
    }

    return result;
}

In the above code we are adding 1 into the variable with inline assembly and then returning the result. The variable result will overflow and 0 will be returned, despite this the contract will not throw an error or revert.

Use of unchecked code block:

Performing arithmetic operations inside the unchecked block saves a lot of gas because it omits several checks and opcodes. But, some of these opcodes are used in default arithmetic operations in 0.8 to check for underflow/overflow.

uint8 public a = 255;

function uncheck() public{

  unchecked {
      a++;  // overflow and reset to 0 without reverting
  }

}

The unchecked code block is only recommended if you are sure that there is no possible way for the arithmetic to overflow or underflow.

Sources

Off-By-One

Off-by-one errors are a common mistake made by programmers in which the intended boundaries are incorrect by only one, though these errors may seem insignificant, the effect can easily be quite severe.

Array lengths

Properly determining intended array lengths is a common source of off-by-one errors. Particularly since 0-indexing means the final value in an array is array.length - 1.

Consider for example a function intended to loop over a list of recipients to transfer funds to each user, but the loop length is incorrectly set.

// Incorrectly sets upper bound to users.length - 1
// Final user in array doesn't receive token transfer
for (uint256 i; i < users.length - 1; ++i) {
	token.transfer(users[i], 1 ether);
}

Incorrect comparison operator

It's common for comparison operators to be off by one when, e.g. > should be used in place of >=. This is especially common when the logic includes some kind of negation, leading to mental friction in deciphering the intended vs implemented bounds.

Consider for example a Defi protocol with liquidation logic documented to liquidate a user only if their collateralization ratio is below 1e18.

// Incorrectly liquidates if collateralizationRatio is == 1 ether
if (collateralizationRatio > 1 ether) {
	...
} else {
	liquidate();
}

Sources

Lack of Precision

In Solidity, there are a limited variety of number types. Differently from many programming languages, floating point numbers are unsupported. Fixed point numbers are partially supported, but cannot be assigned to or from. The primary number type in Solidity are integers, of which resulting values of calculations are always rounded down.

Since division often results in a remainder, performing division with integers generally requires a lack of precision to some degree. To see how a lack of precision may cause a serious flaw, consider the following example in which we charge a fee for early withdrawals denominated in the number of days early that the withdrawal is made:

uint256 daysEarly = withdrawalsOpenTimestamp - block.timestamp / 1 days
uint256 fee = amount / daysEarly * dailyFee

The problem with this is that in the case that a user withdraws 1.99 days early, since 1.99 will round down to 1, the user only pays about half the intended fee.

In general, we should ensure that numerators are sufficiently larger than denominators to avoid precision errors. A common solution to this problem is to use fixed point logic, i.e. raising integers to a sufficient number of decimals such that the lack of precision has minimal effect on the contract logic. A good rule of thumb is to raise numbers to 1e18 (commonly referred to as WAD).

Reentrancy

Reentrancy is an attack that can occur when a bug in a contract may allow a malicious contract to reenter the contract unexpectedly during execution of the original function. This can be used to drain funds from a smart contract if used maliciously. Reentrancy is likely the single most impactful vulnerability in terms of total loss of funds by smart contract hacks, and should be considered accordingly. List of reentrancy attacks

External calls

Reentrancy can be executed by the availability of an external call to an attacker controlled contract. External calls allow for the callee to execute arbitrary code. The existence of an external call may not always be obvious, so it's important to be aware of any way in which an external call may be executed in your smart contracts.

ETH transfers

When Ether is transferred to a contract address, it will trigger the receive or fallback function, as implemented in the contract. An attacker can write any arbitrary logic into the fallback method, such that anytime the contract receives a transfer, that logic is executed.

safeMint

One example of a hard to spot external call is OpenZeppelin's ERC721._safeMint & ERC721._safeTransfer functions.

/**
  * @dev Same as {xref-ERC721-_safeMint-address-uint256-}[`_safeMint`], with an additional `data` parameter which is
  * forwarded in {IERC721Receiver-onERC721Received} to contract recipients.
  */
function _safeMint(
    address to,
    uint256 tokenId,
    bytes memory _data
) internal virtual {
    _mint(to, tokenId);
    require(
        _checkOnERC721Received(address(0), to, tokenId, _data),
        "ERC721: transfer to non ERC721Receiver implementer"
    );
}

The function is titled _safeMint because it prevents tokens from being unintentionally minted to a contract by checking first whether that contract has implemented ERC721Receiver, i.e. marking itself as a willing recipient of NFTs. This all seems fine, but _checkOnERC721Received is an external call to the receiving contract, allowing arbitrary execution.

Single function reentrancy

A single function reentrancy attack occurs when a vulnerable function is the same function that an attacker is trying to recursively call.

// UNSECURE
function withdraw() external {
    uint256 amount = balances[msg.sender];
    (bool success,) = msg.sender.call{value: balances[msg.sender]}("");
    require(success);
    balances[msg.sender] = 0;
}

Here we can see that the balance is only modified after the funds have been transferred. This can allow a hacker to call the function many times before the balance is set to 0, effectively draining the smart contract.

Cross-function reentrancy

A cross-function reentrancy attack is a more complex version of the same process. Cross-function reentrancy occurs when a vulnerable function shares state with a function that an attacker can exploit.

// UNSECURE
function transfer(address to, uint amount) external {
  if (balances[msg.sender] >= amount) {
    balances[to] += amount;
    balances[msg.sender] -= amount;
  }
}

function withdraw() external {
  uint256 amount = balances[msg.sender];
  (bool success,) = msg.sender.call{value: balances[msg.sender]}("");
  require(success);
  balances[msg.sender] = 0;
}

In this example, a hacker can exploit this contract by having a fallback function call transfer() to transfer spent funds before the balance is set to 0 in the withdraw() function.

Read-only Reentrancy

Read-only reentrancy is a novel attack vector in which instead of reentering into the same contract in which state changes have yet to be made, an attacker reenters into another contract which reads from the state of the original contract.

// UNSECURE
contract A {
	// Has a reentrancy guard to prevent reentrancy
	// but makes state change only after external call to sender
	function withdraw() external nonReentrant {
		uint256 amount = balances[msg.sender];
		(bool success,) = msg.sender.call{value: balances[msg.sender]}("");
		require(success);
		balances[msg.sender] = 0;
	}
}

contract B {
	// Allows sender to claim equivalent B tokens for A tokens they hold
	function claim() external nonReentrant {
		require(!claimed[msg.sender]);
		balances[msg.sender] = A.balances[msg.sender];
		claimed[msg.sender] = true;
	}
}

As we can see in the above example, although both functions have a nonReentrant modifier, it is still possible for an attacker to call B.claim during the callback in A.withdraw, and since the attackers balance was not yet updated, execution succeeds.

Reentrancy prevention

The simplest reentrancy prevention mechanism is to use a ReentrancyGuard, which allows you to add a modifier, e.g. nonReentrant, to functions which may otherwise be vulnerable. Although effective against most forms of reentrancy, it's important to understand how read-only reentrancy may be used to get around this and to always use the checks-effects-interactions pattern.

For optimum security, use the checks-effects-interactions pattern. This is a simple rule of thumb for ordering smart contract functions.

The function should begin with checks, e.g. require and assert statements.

Next, the effects of the contract should be performed, i.e. state modifications.

Finally, we can perform interactions with other smart contracts, e.g. external function calls.

This structure is effective against reentrancy because when an attacker reenters the function, the state changes have already been made. For example:

function withdraw() external {
  uint256 amount = balances[msg.sender];
  balances[msg.sender] = 0;
  (bool success,) = msg.sender.call{value: balances[msg.sender]}("");
  require(success);
}

Since the balance is set to 0 before any interactions are performed, if the contract is called recursively, there is nothing to send after the first transaction.

Examples from: https://medium.com/coinmonks/protect-your-solidity-smart-contracts-from-reentrancy-attacks-9972c3af7c21

Sources

DoS with Block Gas Limit

One of the primary benefits of a block gas limit is that it prevents attackers from creating an infinite transaction loop. If the gas usage of a transaction exceeds this limit, the transaction will fail. However, along with this benefit comes a side effect which is important to understand.

Unbounded Operations

An example in which the block gas limit can be an issue is in executing logic in an unbounded loop. Even without any malicious intent, this can easily go wrong. Just by e.g., having too large an array of users to send funds to can exceed the gas limit and prevent the transaction from ever succeeding, potentially permanently locking up funds.

This situation can also lead to an attack. Say a bad actor decides to create a significant amount of addresses, with each address being paid a small amount of funds from the smart contract. If done effectively, the transaction can be blocked indefinitely, possibly even preventing further transactions from going through.

An effective solution to this problem would be to use a pull payment system over the above push payment system. To do this, separate each payment into its own transaction, and have the recipient call the function.

If, for some reason, you really need to loop through an array of unspecified length, at least expect it to potentially take multiple blocks, and allow it to be performed in multiple transactions - as seen in this example:

struct Payee {
    address addr;
    uint256 value;
}

Payee[] payees;
uint256 nextPayeeIndex;

function payOut() {
    uint256 i = nextPayeeIndex;
    while (i < payees.length && msg.gas > 200000) {
      payees[i].addr.send(payees[i].value);
      i++;
    }
    nextPayeeIndex = i;
}

Block Stuffing

In some situations, your contract can be attacked with a block gas limit even if you don't loop through an array of unspecified length. An attacker can fill several blocks before a transaction can be processed by using a sufficiently high gas price.

This attack is done by issuing several transactions at a very high gas price. If the gas price is high enough, and the transactions consume enough gas, they can fill entire blocks and prevent other transactions from being processed.

Ethereum transactions require the sender to pay gas to disincentivize spam attacks, but in some situations, there can be enough incentive to go through with such an attack. For example, a block stuffing attack was used on a gambling Dapp, Fomo3D. The app had a countdown timer, and users could win a jackpot by being the last to purchase a key, except every time a user bought a key, the timer would be extended. An attacker bought a key then stuffed the next 13 blocks in a row so they could win the jackpot.

To prevent such attacks from occurring, it's important to carefully consider whether it's safe to incorporate time-based actions in your application.

Example from: https://consensys.github.io/smart-contract-best-practices/attacks/denial-of-service/

Sources

DoS with (Unexpected) revert

A DoS (Denial of Service) may be caused when logic is unable to be executed as a result of an unexpected revert. This can happen for a number of reasons and it's important to consider all the ways in which your logic may revert. The examples listed below are non-exhaustive.

Reverting funds transfer

DoS (Denial of Service) attacks can occur in functions when you try to send funds to a user and the functionality relies on that fund transfer being successful.

This can be problematic in the case that the funds are sent to a smart contract created by a bad actor, since they can simply create a fallback function that reverts all payments.

For example:

// INSECURE
contract Auction {
    address currentLeader;
    uint highestBid;

    function bid() payable {
        require(msg.value > highestBid);

        require(currentLeader.send(highestBid)); // Refund the old leader, if it fails then revert

        currentLeader = msg.sender;
        highestBid = msg.value;
    }
}

As you can see in this example, if an attacker bids from a smart contract with a fallback function reverting all payments, they can never be refunded, and thus no one can ever make a higher bid.

This can also be problematic without an attacker present. For example, you may want to pay an array of users by iterating through the array, and of course you would want to make sure each user is properly paid. The problem here is that if one payment fails, the function is reverted and no one is paid.

address[] private refundAddresses;
mapping (address => uint) public refunds;

// bad
function refundAll() public {
    for(uint x; x < refundAddresses.length; x++) { // arbitrary length iteration based on how many addresses participated
        require(refundAddresses[x].send(refunds[refundAddresses[x]])) // doubly bad, now a single failure on send will hold up all funds
    }
}

An effective solution to this problem would be to use a pull payment system over the above push payment system. To do this, separate each payment into its own transaction, and have the recipient call the function.

contract auction {
    address highestBidder;
    uint highestBid;
    mapping(address => uint) refunds;

    function bid() payable external {
        require(msg.value >= highestBid);

        if (highestBidder != address(0)) {
            refunds[highestBidder] += highestBid; // record the refund that this user can claim
        }

        highestBidder = msg.sender;
        highestBid = msg.value;
    }

    function withdrawRefund() external {
        uint refund = refunds[msg.sender];
        refunds[msg.sender] = 0;
        (bool success, ) = msg.sender.call.value(refund)("");
        require(success);
    }
}

Examples from: https://consensys.github.io/smart-contract-best-practices/attacks/denial-of-service/ https://consensys.github.io/smart-contract-best-practices/development-recommendations/general/external-calls/

Over/Underflow

Prior to SafeMath usage, whether built-in in solidity >=0.8.0 or using a library, over/underflows would result in rolling over to the minimum/maximum value. Now that checked math is commonplace, it's important to recognize that the effect of checked under/overflows is a revert, which may DoS important logic.

Regardless of usage of checked math, it's necessary to ensure that any valid input will not result in an over/underflow. Take extra care when working with smaller integers e.g. int8/uint8, int16/uint16, int24/uint24, etc..

Unexpected Balance

It's important to take caution in enforcing expected contract balances of tokens or Ether as those balances may be increased by an attacker to cause an unexpected revert. This is easily possible with ERC20 tokens by simply transferring to the contract, but is also possible with Ether by forcibly sending Ether to a contract.

Consider, for example, a contract which expects the Ether balance to be 0 for the first deposit to allow for custom accounting logic. An attacker may forcibly send Ether to the contract before the first deposit, causing all deposits to revert.

Divide by Zero

In solidity if the contract attempts to perform division when the denominator is zero, the call reverts. Thus, the denominator should be always checked before division to prevent DoS revert.

function foo(uint num, uint den) public pure returns(uint result) {
  result = num / den; // if den = 0, the execution reverts
}

Sources

Using msg.value in a Loop

The value of msg.value in a transaction’s call never gets updated, even if the called contract ends up sending some or all of the ETH to another contract. This means that using msg.value in for or while loops, without extra accounting logic, will either lead to the transaction reverting (when there are no longer sufficient funds for later iterations), or to the contract being drained (when the contract itself has an ETH balance).

contract depositer {
    function deposit(address weth) payable external {
        for (uint i = 0; i < 5; i ++) {
            WETH(weth).deposit{value: msg.value}();
        }
    }
}

In the above example, first iteration will use all the msg.value for the external call and all other iterations can:

  • Drain the contract if enough ETH balance exists inside the contract to cover all the iterations.
  • Revert if enough ETH balance doesn't exist inside the contract to cover all the iterations.
  • Succeed if the external implementation succeeds with zero value transfers.

Also, if a function has a check like require(msg.value == 1e18, "Not Enough Balance"), that function can be called multiple times in a same transaction by sending 1 ether once as msg.value is not updated in a transaction call.

function batchBuy(address[] memory addr) external payable{
    mapping (uint => address) nft;

    for (uint i = 0; i < addr.length; i++) {
         buy1NFT(addr[i])
    }

    function buy1NFT(address to) internal {
         if (msg.value < 1 ether) { // buy unlimited times after sending 1 ether once
            revert("Not enough ether");
            } 
         nft[numero] = address;
    }
}

Thus, using msg.value inside a loop is dangerous because this might allow the sender to re-use the msg.value.

Reuse of msg.value can also show up with payable multicalls. Multicalls enable a user to submit a list of transactions to avoid paying the 21,000 gas transaction fee over and over. However, If msg.value gets re-used while looping through the functions to execute, it can cause a serious issue like the Opyn Hack.

Sources

Transaction-Ordering Dependence

Transactions on Ethereum are grouped together in blocks which are processed on a semi-regular interval, 12 seconds. Before transactions are placed in blocks, they are broadcasted to the mempool where block builders can then proceed to place them as is economically optimal. What's important to understand here is that the mempool is public and thus anyone can see transactions before they're executed, giving them the power to frontrun by placing their own transaction executing the same, or a similar, action with a higher gas price.

Frontrunning has become prevalent as a result of generalized frontrunning bots becoming more and more common. These generalized frontrunners work by observing the mempool for profitable, replicable transactions which they can replace for their own benefit. Ethereum is a Dark Forest.

One solution to transaction-ordering dependence is to use a commit-reveal scheme in the case of information being submitted on-chain. This works by having the submitter send in a hash of the information, storing that on-chain along with the user address so that they may later reveal the answer along with the salt to prove that they were indeed correct. Another solution is to simply use a private mempool such as Flashbots.

Sources

Insufficient Gas Griefing

Insufficient gas griefing can be done on contracts which accept data and use it in a sub-call on another contract. This method is often used in multisignature wallets as well as transaction relayers. If the sub-call fails, either the whole transaction is reverted, or execution is continued.

Let's consider a simple relayer contract as an example. As shown below, the relayer contract allows someone to make and sign a transaction, without having to execute the transaction. Often this is used when a user can't pay for the gas associated with the transaction.

contract Relayer {
    mapping (bytes => bool) executed;

    function relay(bytes _data) public {
        // replay protection; do not call the same transaction twice
        require(executed[_data] == 0, "Duplicate call");
        executed[_data] = true;
        innerContract.call(bytes4(keccak256("execute(bytes)")), _data);
    }
}

The user who executes the transaction, the 'forwarder', can effectively censor transactions by using just enough gas so that the transaction executes, but not enough gas for the sub-call to succeed.

There are two ways this could be prevented. The first solution would be to only allow trusted users to relay transactions. The other solution is to require that the forwarder provides enough gas, as seen below.

// contract called by Relayer
contract Executor {
    function execute(bytes _data, uint _gasLimit) {
        require(gasleft() >= _gasLimit);
        ...
    }
}

Sources

Unchecked Return Values

The main idea behind this type of vulnerability is the failure to properly handle the return values of external function calls. This can have significant consequences, including fund loss and unexpected behavior in the contract's logic.

In Solidity, developers can perform external calls using methods like:

  1. .send()
  2. .call()
  3. .transfer()

.transfer() is commonly used to send ether to external accounts, however, the .send() function can also be used. For more versatile external calls, .call() can be used.

Each of these methods has a different behavior when it comes to error handling. The .call() and .send() functions return a boolean indicating if the call succeeded or failed. Thus, these functions have a simple caveat: the transaction that executes these functions (.call() and .send()) WILL NOT revert if the external call fails. Instead, .call() and .send() will simply return the boolean value false.

A common pitfall arises when the return value is not checked, as the developer expects a revert to occur when, in reality, the revert will not occur if not explicitly checked by the smart contract.

For example, if a contract uses .send() without checking its return value, transaction execution will continue even if the call fails, resulting in unexpected behavior. Take the below contract for example:

/// INSECURE
contract Lotto {

    bool public paidOut = false;
    address public winner;
    uint256 public winAmount;

    /// extra functionality here

    function sendToWinner() public {
        require(!paidOut);
        winner.send(winAmount);
        paidOut = true;
    }

    function withdrawLeftOver() public {
        require(paidOut);                // requires `paidOut` to be true
        msg.sender.send(this.balance);
    }
}

The above contract represents a Lotto-like contract, where a winner receives winAmount of ether, which typically leaves a little left over for anyone to withdraw.

The bug exists where .send() is used without checking the response, i.e., winner.send(winAmount).

In this example, a winner whose transaction fails (either by running out of gas or being a contract that intentionally throws in the fallback function) will still allow paidOut to be set to true (regardless of whether ether was sent or not).

In this case, anyone can withdraw the winner's winnings using the withdrawLeftOver() function.

A more serious version of this bug occurred in King of the Ether. An excellent post-mortem of this contract has been written, detailing how an unchecked failed .send() could be used to attack a contract.

Preventive Techniques

To mitigate this vulnerability, developers should always check the return value of any call to an external contract. The require() function can be used to check if the call was successful and handle any errors that may occur.

A caveat developers should be wary of when using the require() function is unexpected reverts that can cause DoS. If the developer naively decides to check for the success or failure of the external .send() call like so:

/// INSECURE
contract Lotto {

    bool public paidOut = false;
    address public winner;
    uint256 public winAmount;

    /// extra functionality here

  function sendToWinner() public {
        require(!paidOut);
        require(winner.send(winAmount));        // naively check success of the external call
        paidOut = true;
    }

  function withdrawLeftOver() public {
        require(paidOut);                       // requires `paidOut` to be true
        msg.sender.send(this.balance);
    }

An attacker interacting with the Lotto contract from their own malicious contract and calling the sendToWinner function, can just implement a fallback function that reverts all payments making paidOut not set to true!

A detailed explanation of this caveat can be found here

Sources

Write to Arbitrary Storage Location

Only authorized addresses should have access to write to sensitive storage locations. If there isn't proper authorization checks throughout the contract, a malicious user may be able to overwrite sensitive data. However, even if there are authorization checks for writing to sensitive data, an attacker may still be able to overwrite the sensitive data via insensitive data. This could give an attacker access to overwrite important variables such as the contract owner.

To prevent this from occurring, we not only want to protect sensitive data stores with authorization requirements, but we also want to ensure that writes to one data structure cannot inadvertently overwrite entries of another data structure.

For an example, try Ethernaut - Alien Codex. If it's too hard, see this walkthrough (SPOILER).

Sources

Unbounded Return Data

The Byzantium 2017 mainnet hard-fork introduced EIP-211. This EIP established an arbitrary-length return data buffer as well as 2 new opcodes: RETURNDATASIZE and RETURNDATACOPY. This enables callers to copy all or part of the return data from an external call to memory. The variable length buffer is created empty for each new call-frame. Previously, the size of the return data had to be specified in advance in the call parameters.

However under Solidity's implementation, up until at least 0.8.26, the entirety of this return data is automatically copied from the buffer into memory. This is true even when using a Solidity low-level call with the omission of the bytes memory data syntax.

Consider the following example:

pragma solidity 0.8.26;

contract Attacker {
    function returnExcessData() external pure returns (string memory) {
        revert("Passing in excess data that the Solidity compiler will automatically copy to memory");   // Both statements can return unbounded data
        return "Passing in excess data that the Solidity compiler will automatically copy to memory";
    }
}


contract Victim {
    function callAttacker(address attacker) external returns (bool) {
        (bool success, ) = attacker.call{gas: 2500}(abi.encodeWithSignature("returnExcessData()"));
        return success;
    }
}

In the above example one can observe that even though the Victim contract has not explicitly requested bytes memory data to be returned, and has furthermore given the external call a gas stipend of 2500, Solidity will still invoke RETURNDATACOPY during the top-level call-frame. This means the Attacker contract, through revert or return, can force the Victim contract to consume unbounded gas during their own call-frame and not that of the Attacker. Given that memory gas costs grow exponentially after 23 words, this attack vector has the potential to prevent certain contract flows from being executed due to an Out of Gas error. Examples of vulnerable contract flows include unstaking or undelegating funds where a callback is involved. Here the user may be prevented from unstaking or undelegating their funds, because the transaction reverts due to insufficient gas.

Mitigation

The recommended mitigation approach is to use Yul to make the low-level call, whilst only allowing bounded return data. This method completely cuts off the attack vector for any arbitrary external call.

Consider the following example from EigenLayer's original mainnet DelegationManager.sol contract. In this contract, delegators could delegate and undelegate their restaked assets to a manager, and each of these delegation flows had its own callback hook to an arbitrary external contract the manager specified. However the manager could use their arbitrary external contract to return unbounded data, causing the delegator to run out of gas, and thus not be able to undelegate their assets from that manager.

Therefore to mitigate this griefing risk entirely, EigenLayer used a Yul call, where they limit the return data size to 1 word. If the external manager contract tries to return any more data than this, the excess of 32 bytes simply won't be copied to memory.

    function _delegationWithdrawnHook(
        IDelegationTerms dt,
        address staker,
        IStrategy[] memory strategies,
        uint256[] memory shares
    )
        internal
    {
        /**
         * We use low-level call functionality here to ensure that an operator cannot maliciously make this function fail in order to prevent undelegation.
         * In particular, in-line assembly is also used to prevent the copying of uncapped return data which is also a potential DoS vector.
         */
        // format calldata
        bytes memory lowLevelCalldata = abi.encodeWithSelector(IDelegationTerms.onDelegationWithdrawn.selector, staker, strategies, shares);
        // Prepare memory for low-level call return data. We accept a max return data length of 32 bytes
        bool success;
        bytes32[1] memory returnData;
        // actually make the call
        assembly {
            success := call(
                // gas provided to this context
                LOW_LEVEL_GAS_BUDGET,
                // address to call
                dt,
                // value in wei for call
                0,
                // memory location to copy for calldata
                add(lowLevelCalldata, 32),
                // length of memory to copy for calldata
                mload(lowLevelCalldata),
                // memory location to copy return data
                returnData,
                // byte size of return data to copy to memory
                32
            )
        }
        // if the call fails, we emit a special event rather than reverting
        if (!success) {
            emit OnDelegationWithdrawnCallFailure(dt, returnData[0]);
        }
    }

Sources

Uninitialized Storage Pointer

[!NOTE]
As of solidity 0.5.0, uninitialized storage pointers are no longer an issue since contracts with uninitialized storage pointers will no longer compile. This being said, it's still important to understand what storage pointers you should be using in certain situations.

Data is stored in the EVM as either storage, memory, or calldata. It is important that they are well understood and correctly initialized. Incorrectly initializing data storage pointers, or simply leaving them uninitialized, can lead to contract vulnerabilities.

Sources

Unexpected ecrecover Null Address

ecrecover is a precompiled built-in cryptographic function which recovers an address associated with the public key from an elliptic curve signature or returns zero on error. The parameters corresponding to the signature are r, s & v.

As noted above, ecrecover will return zero on error. It's possible to do this deterministically by setting v as any positive number other than 27 or 28.

This can be manipulated by attackers to make it seem like a valid message has been signed by address(0).

NOTE: The default value for addresses in solidity is address(0). As such, in case important storage variables, e.g. owner/admin, are unset, it's possible to spoof a signature from one of these unset addresses, executing authorized-only logic.

// UNSECURE
function validateSigner(address signer, bytes32 message, uint8 v, bytes32 r, bytes32 s) internal pure returns (bool) {
	address recoveredSigner = ecrecover(message, v, r, s);
	return signer == recoveredSigner;
}

The above method is intended to only return true if a valid signature is provided. However, as we know, if we set v to any value other than 27 or 28, the recoveredSigner will be the null address and if the provided signer is address(0), the function will unexpectedly return true.

We can mitigate this issue by reverting if the recoveredSigner address is null, e.g.:

function validateSigner(address signer, bytes32 message, uint8 v, bytes32 r, bytes32 s) internal pure returns (bool) {
        address recoveredSigner = ecrecover(message, v, r, s);
        require(recoveredSigner != address(0));
        return signer == recoveredSigner;
}

Sources

Weak Sources of Randomness from Chain Attributes

Using chain attributes for randomness, e.g.: block.timestamp, blockhash, and block.difficulty can seem like a good idea since they often produce pseudo-random values. The problem, however, is that Ethereum is entirely deterministic and all available on-chain data is public. Chain attributes can either be predicted or manipulated, and should thus never be used for random number generation.

Example of Weak Randomness

pragma solidity ^0.8.24;

contract GuessTheRandomNumber {
    constructor() payable {}

    function guess(uint256 _guess) public {
        uint256 answer = uint256(
            keccak256(
                abi.encodePacked(blockhash(block.number - 1), block.timestamp)
            )
        );

        if (_guess == answer) {
            (bool sent,) = msg.sender.call{value: 1 ether}("");
            require(sent, "Failed to send Ether");
        }
    }
}

In the above example, the answer variable is initialized using blockhash(block.number - 1) and block.timestamp. This method is insecure because both blockhash and block.timestamp can be retrieved directly by another contract just in time, making it possible to guess the answer and win the challenge unfairly.

An attacker can exploit the weak randomness as follows:

contract Attack {
    receive() external payable {}

    function attack(GuessTheRandomNumberChallenge guessTheRandomNumber) public {
        uint256 answer = uint256(
            keccak256(
                abi.encodePacked(blockhash(block.number - 1), block.timestamp)
            )
        );

        guessTheRandomNumber.guess(uint8(answer));
    }

    // Helper function to check balance
    function getBalance() public view returns (uint256) {
        return address(this).balance;
    }
}

The Attack contract calculates the answer using the same logic as the GuessTheRandomNumber contract and guesses it correctly, allowing the attacker to win the challenge.

Preventive Measures

A common and more secure solution is to use an oracle service such as Chainlink VRF, which provides verifiable randomness that cannot be manipulated.

Sources

Hash Collision when using abi.encodePacked() with Multiple Variable-Length Arguments

In Solidity, the abi.encodePacked() function is used to create tightly packed byte arrays which can then be hashed using keccak256()

However, this function can be dangerous when used with multiple variable-length arguments because it can lead to hash collisions. These collisions can potentially be exploited in scenarios such as signature verification, allowing attackers to bypass authorization mechanisms.

Hash Collision is a situation where two different sets of inputs produce the same hash output. In this context, a hash collision can occur when using abi.encodePacked() with multiple variable-length arguments, allowing an attacker to craft different inputs that produce the same hash.

Understanding the vulnerability

When abi.encodePacked() is used with multiple variable-length arguments (such as arrays and strings), the packed encoding does not include information about the boundaries between different arguments. This can lead to situations where different combinations of arguments result in the same encoded output, causing hash collisions.

For example, consider the following two calls to abi.encodePacked():

abi.encodePacked(["a", "b"], ["c", "d"])

abi.encodePacked(["a"], ["b", "c", "d"])

Both calls could potentially produce the same packed encoding because abi.encodePacked() simply concatenates the elements without any delimiters!

Consider the below example for strings:

abi.encodePacked("foo", "bar") == abi.encodePacked("fo", "obar")

Strings in Solidity are dynamic types and when they are concatenated using abi.encodePacked(), there is no delimiter between them to mark their boundaries, which can lead to hash collisions.

As a matter of fact, the below warning is taken as it is straight from the official solidity language documentation regarding the same.

[!WARNING] If you use keccak256(abi.encodePacked(a, b)) and both a and b are dynamic types, it is easy to craft collisions in the hash value by moving parts of a into b and vice-versa. More specifically, abi.encodePacked("a", "bc") == abi.encodePacked("ab", "c"). If you use abi.encodePacked for signatures, authentication or data integrity, make sure to always use the same types and check that at most one of them is dynamic. Unless there is a compelling reason, abi.encode should be preferred.

Sample Code Analysis

/// INSECURE
function addUsers(address[] calldata admins, address[] calldata regularUsers, bytes calldata signature) external {
    if (!isAdmin[msg.sender]) {
        bytes32 hash = keccak256(abi.encodePacked(admins, regularUsers));
        address signer = hash.toEthSignedMessageHash().recover(signature);
        require(isAdmin[signer], "Only admins can add users.");
    }
    for (uint256 i = 0; i < admins.length; i++) {
        isAdmin[admins[i]] = true;
    }
    for (uint256 i = 0; i < regularUsers.length; i++) {
        isRegularUser[regularUsers[i]] = true;
    }
}

In the provided sample code above, the addUsers function uses abi.encodePacked(admins, regularUsers) to generate a hash. An attacker could exploit this by rearranging elements between the admins and regularUsers arrays, resulting in the same hash and thereby bypassing authorization checks.

/// INSECURE
function verifyMessage(string calldata message1, string calldata message2, bytes calldata signature) external {
    bytes32 hash = keccak256(abi.encodePacked(message1, message2));
    address signer = hash.toEthSignedMessageHash().recover(signature);
    require(isAuthorized[signer], "Unauthorized signer");
}

The above function verifyMessage() could easily be exploited as below:-

verifyMessage("hello", "world", signature);

or

verifyMessage("hell", "oworld", signature);

or a variation of the string hello world

All variations of the string hello world passed to verifyMessage() would produce the same hash, potentially allowing an attacker to bypass the authorization check if they can provide a valid signature for their manipulated inputs.

Fixed Code Using Single User:

function addUser(address user, bool admin, bytes calldata signature) external {
    if (!isAdmin[msg.sender]) {
        bytes32 hash = keccak256(abi.encodePacked(user));
        address signer = hash.toEthSignedMessageHash().recover(signature);
        require(isAdmin[signer], "Only admins can add users.");
    }
    if (admin) {
        isAdmin[user] = true;
    } else {
        isRegularUser[user] = true;
    }
}

This approach eliminates the use of variable-length arrays, thus avoiding the hash collision issue entirely by dealing with a single user at a time.

Fixed Code Using Fixed-Length Arrays:

function addUsers(address[3] calldata admins, address[3] calldata regularUsers, bytes calldata signature) external {
    if (!isAdmin[msg.sender]) {
        bytes32 hash = keccak256(abi.encodePacked(admins, regularUsers));
        address signer = hash.toEthSignedMessageHash().recover(signature);
        require(isAdmin[signer], "Only admins can add users.");
    }
    for (uint256 i = 0; i < admins.length; i++) {
        isAdmin[admins[i]] = true;
    }
    for (uint256 i = 0; i < regularUsers.length; i++) {
        isRegularUser[regularUsers[i]] = true;
    }
}

In this version, fixed-length arrays are used, which mitigates the risk of hash collisions since the encoding is unambiguous.

Remediation Strategies

To prevent this type of hash collision, the below remediation strategies can be employed:

  1. Avoid Variable-Length Arguments: Avoid using abi.encodePacked() with variable-length arguments such as arrays and strings. Instead, use fixed-length arrays to ensure the encoding is unique and unambiguous.

  2. Use abi.encode() Instead: Unlike abi.encodePacked(), abi.encode() includes additional type information and length prefixes in the encoding, making it much less prone to hash collisions. Switching from abi.encodePacked() to abi.encode() is a simple yet effective fix.

[!IMPORTANT] Replay Protection does not protect against possible hash collisions!

It is listed here as a defense in depth strategy and SHOULD NOT be solely relied upon to protect against said vulnerability

  1. Replay Protection: Implement replay protection mechanisms to prevent attackers from reusing valid signatures. This can involve including nonces or timestamps in the signed data. However, this does not completely eliminate the risk of hash collisions but adds an additional layer of security. More on this can be found here

Sources

Timestamp Dependence

NOTE: This vulnerability no longer affects Ethereum mainnet as of the Proof of Stake merge. Read more

The timestamp of a block, accessed by block.timestamp or alias now can be manipulated by a miner. There are three considerations you should take into account when using a timestamp to execute a contract function.

Timestamp Manipulation

If a timestamp is used in an attempt to generate randomness, a miner can post a timestamp within 15 seconds of block validation, giving them the ability to set the timestamp as a value that would increase their odds of benefitting from the function.

For example, a lottery application may use the block timestamp to pick a random bidder in a group. A miner may enter the lottery then modify the timestamp to a value that gives them better odds at winning the lottery.

Timestamps should thus not be used to create randomness. See Weak Sources of Randomness for Chain Attributes.

The 15-second Rule

Ethereum's reference specification, the Yellow Paper, doesn't specify a limit as to how much blocks can change in time, it just has to be bigger than the timestamp of its parent. This being said, popular protocol implementations reject blocks with timestamps greater than 15 seconds in the future, so as long as your time-dependent event can safely vary by 15 seconds, it may be safe to use a block timestamp.

Don't use block.number as a timestamp

You can estimate the time difference between events using block.number and the average block time, but block times may change and break the functionality, so it's best to avoid this use.

Sources

Unsafe Low-Level Call

In Solidity, you can either use low-level calls such as: address.call(), address.callcode(), address.delegatecall(), and address.send(); or you can use contract calls such as: ExternalContract.doSomething().

Low-level calls can be a good way to efficiently or arbitrarily make contract calls. However, it's important to be aware of the caveats it possesses.

Unchecked call return value

Low-level calls will never throw an exception, instead they will return false if they encounter an exception, whereas contract calls will automatically throw. Thus if the return value of a low-level call is not checked, the execution may resume even if the function call throws an error. This can lead to unexpected behaviour and break the program logic. A failed call can even be caused intentionally by an attacker, who may be able to further exploit the application.

In the case that you use low-level calls, be sure to check the return value to handle possible failed calls, e.g.:

// Simple transfer of 1 ether
(bool success,) = to.call{value: 1 ether}("");
// Revert if unsuccessful
require(success);

Successful call to non-existent contract

As noted in the Solidity docs: "Due to the fact that the EVM considers a call to a non-existing contract to always succeed, Solidity uses the extcodesize opcode to check that the contract that is about to be called actually exists (it contains code) and causes an exception if it does not. This check is skipped if the return data will be decoded after the call and thus the ABI decoder will catch the case of a non-existing contract.

Note that this check is not performed in case of low-level calls which operate on addresses rather than contract instances."

It's imperative that we do not simply assume that a contract to be called via a low-level call actually exists, since if it doesn't our logic will proceed even though our external call effectively failed. This can lead to loss of funds and/or an invalid contract state. Instead, we must verify that the contract being called exists, either immediately before being called with an extcodesize check, or by verifying during contract deployment and using a constant/immutable value if the contract can be fully trusted.

// Verify address is a contract
require(to.code.length > 0);
// Simple transfer of 1 ether
(bool success,) = to.call{value: 1 ether}("");
// Revert if unsuccessful
require(success);

Sources

Unsupported Opcodes

EVM-compatible chains, such as zkSync Era, BNB Chain, Polygon, Optimism and Arbitrum implement the Ethereum Virtual Machine (EVM) and its opcodes. However, opcode support can vary across these chains, which can lead to bugs and issues if not considered during smart contract development and deployment.

PUSH0 Opcode Compatibility

The PUSH0 opcode was introduced during the Shanghai hard fork of the Shapella upgrade (Solidity v0.8.20) and is available in certain EVM-compatible chains. However, not all chains have implemented support for this opcode yet.

Is PUSH0 supported:

  1. Ethereum: YES
  2. Arbitrum One: YES
  3. Optimism: YES
  4. ...

More chain differences and opcode support can be found on: evmdiff.com

You can also check compatibility by running the following command assuming you have Foundry set up:

cast call --rpc-url $ARBITRUM_RPC_URL --create 0x5f

Getting a 0x response from running the above command means the opcode is supported; an error indicates the opcode isn't supported on that chain.

CREATE and CREATE2 on zkSync Era

On zkSync Era, contract deployment uses the hash of the bytecode and the factoryDeps field of EIP712 transactions contains the bytecode. The actual deployment occurs by providing the contract's hash to the ContractDeployer system contract.

To ensure that create and create2 functions operate correctly, the compiler MUST be aware of the bytecode of the deployed contract in advance. The compiler interprets the calldata arguments as incomplete input for ContractDeployer, with the remaining part filled in by the compiler internally. The Yul datasize and dataoffset instructions have been adjusted to return the constant size and bytecode hash rather than the bytecode itself.

The following code will not function correctly because the compiler is not aware of the bytecode beforehand but will work fine on Ethereum Mainnet:

function myFactory(bytes memory bytecode) public {
   assembly {
      addr := create(0, add(bytecode, 0x20), mload(bytecode))
   }
}

.transfer() on zkSync Era

The transfer() function in Solidity is limited to 2300 gas, which can be insufficient if the receiving contract's fallback or receive function involves more complex logic. This can lead to the transaction reverting if the gas limit is exceeded.

It is for this exact reason that the Gemholic project on zkSync Era locked its 921 ETH that was raised during the Gemholic token sale making the funds inaccessible.

This was because the contract deployment did not account for zkSync Era's handling of the .transfer() function.

Sources

Unencrypted Private Data On-Chain

Ethereum smart contract code, storage, and any data transacted on-chain can always be read. Treat it as such. Even if your code is not verified on Etherscan, attackers can still decompile or check transactions to and from it to analyze it. For this reason, it's imperative that private data is never stored on-chain unencrypted.

Example

Let's consider a scenario where players participate in an Odd or Even game. Each player submits a number, and the winner is determined by the sum of both numbers being odd or even. Here is a vulnerable implementation:

// Vulnerable contract storing unencrypted private data
contract OddEven {
    struct Player { 
        address payable addr; 
        uint number;
    }
   
    Player[2] private players;
    uint8 count = 0; 

    function play(uint number) public payable {
        require(msg.value == 1 ether);
        players[count] = Player(payable(msg.sender), number);
        count++;
        if (count == 2) selectWinner();
    }
   
    function selectWinner() private {
        uint n = players[0].number + players[1].number;
        players[n % 2].addr.transfer(address(this).balance);
        delete players;
        count = 0;
    }
}

In this contract, the players array stores the submitted numbers in plain text. Although the players array is marked as private, this only means it is not accessible via other smart contracts. However, anyone can read the blockchain and view the stored values. This means the first player's number will be visible, allowing the second player to select a number that they know will make them a winner.

Protection Mechanisms

To protect sensitive data, consider the following strategies:

  1. Commit-Reveal Scheme: Use a commit-reveal scheme where the data is committed to the blockchain in one phase and revealed in another.
  2. Zero-Knowledge Proofs: Use cryptographic techniques such as zero-knowledge proofs to validate data without revealing it.

References

Asserting contract from Code Size

A common method for asserting whether a sender is a contract or EOA has been to check the code size of the sender. This check asserts that if the sender has a code size > 0 that it must be a contract and if not then it must be an EOA. For example:

function mint(uint256 amount) public {
  if (msg.sender.code.length != 0) revert CallerNotEOA();
}

However, as noted in the Ethereum Yellow Paper, "During initialization code execution, EXTCODESIZE on the address should return zero, which is the length of the code of the account while CODESIZE should return the length of the initialization".

This repo shows how we may exploit this logic by simply calling during creation of a new contract.

As we can see, it's important that we recognize that although we may be certain that an account with a non-zero codesize is a contract, we can't be certain that an account with a zero codesize is not a contract.

Sources

Floating Pragma

It is considered best practice to pick one compiler version and stick with it. With a floating pragma, contracts may accidentally be deployed using an outdated or problematic compiler version which can cause bugs, putting your smart contract's security in jeopardy. For open-source projects, the pragma also tells developers which version to use, should they deploy your contract. The chosen compiler version should be thoroughly tested and considered for known bugs.

The exception in which it is acceptable to use a floating pragma, is in the case of libraries and packages. Otherwise, developers would need to manually update the pragma to compile locally.

Sources

Outdated Compiler Version

Developers often find bugs and vulnerabilities in existing software and make patches. For this reason, it's important to use the most recent compiler version possible. See bugs from past compiler versions here.

Sources

Use of Deprecated Functions

As time goes by, functions and operators in Solidity are deprecated and often replaced. It's important to not use deprecated functions, as it can lead to unexpected effects and compilation errors.

Here is a non-exhaustive list of deprecated functions and alternatives. Many alternatives are simply aliases, and won't break current behaviour if used as a replacement for its deprecated counterpart.

DeprecatedAlternatives
suicide(address)/selfdestruct(address)N/A
block.blockhash(uint)blockhash(uint)
sha3(...)keccak256(...)
callcode(...)delegatecall(...)
throwrevert()
msg.gasgasleft
constantview
varcorresponding type name

Sources

Incorrect Constructor Name

[!NOTE]
This vulnerability is relevant to older contracts using Solidity versions before 0.4.22. Modern Solidity versions (0.4.22 and later) use the constructor keyword, effectively deprecating this vulnerability. However, it is still important to be aware of this issue when reviewing or interacting with legacy contracts.

Before Solidity 0.4.22, the only way to define a constructor was by creating a function with the contract name. In some cases this was problematic. For example, if a smart contract is re-used with a different name but the constructor function isn't also changed it simply becomes a regular, callable function. Similarly, it's possible for an attacker to create a contract with which a function appears to be the constructor but actually has one character replaced with a similar looking character, e.g. replacing an "l" with a "1", allowing logic to be executed when it's only expected to be executed during contract creation.

Now with modern versions of Solidity, the constructor is defined with the constructor keyword, effectively deprecating this vulnerability. Thus the solution to this problem is simply to use modern Solidity compiler versions.

Sources

Shadowing State Variables

It is possible to use the same variable twice in Solidity, but it can lead to unintended side effects. This is especially difficult regarding working with multiple contracts. Take the following example:

contract SuperContract {
  uint a = 1;
}

contract SubContract is SuperContract {
  uint a = 2;
}

Here we can see that SubContract inherits SuperContract and the variable a is defined twice with different values. Now say we use a to perform some function in SubContract, functionality inherited from SuperContract will no longer work since the value of a has been modified.

To avoid this vulnerability, it's important we check the entire smart contract system for ambiguities. It's also important to check for compiler warnings, as they can flag these ambiguities so long as they're in the smart contract.

Sources

Incorrect Inheritance Order

In Solidity, it is possible to inherit from multiple sources, which if not properly understood can introduce ambiguity. This ambiguity is known as the Diamond Problem, wherein if two base contracts have the same function, which one should be prioritized? Luckily, Solidity handles this problem gracefully, that is as long as the developer understands the solution.

The solution Solidity provides to the Diamond Problem is by using reverse C3 linearization. This means that it will linearize the inheritance from right to left, so the order of inheritance matters. It is suggested to start with more general contracts and end with more specific contracts to avoid problems.

Sources

Presence of Unused Variables

Although it is allowed, it is best practice to avoid unused variables. Unused variables can lead to a few different problems:

  • Increase in computations (unnecessary gas consumption)
  • Indication of bugs or malformed data structures
  • Decreased code readability

It is highly recommended to remove all unused variables from a code base.

Sources

Default Visibility

Visibility specifiers are used to determine where a function or variable can be accessed from. As explained in the solidity docs:

  • public: visible externally and internally (creates a getter function for storage/state variables)
  • private: only visible in the current contract
  • external: only visible externally (only for functions) - i.e. can only be message-called (via this.func)
  • internal: only visible internally

It's important to note that the default visibility is public, allowing access externally or internally by any contract or EOA. We can see how this may be a problem if a method is intended to only be accessible internally but is missing a visibility specifier.

Modern compilers should catch missing function visibility specifiers, but will generally allow missing state variable visibility specifiers. Regardless, it's important to be aware of the possible interactions which may occur as a result of default visibility specifiers for both functions and state variables.

Sources

Inadherence to Standards

In terms of smart contract development, it's important to follow standards. Standards are set to prevent vulnerabilities, and ignoring them can lead to unexpected effects.

Take for example binance's original BNB token. It was marketed as an ERC20 token, but it was later pointed out that it wasn't actually ERC20 compliant for a few reasons:

  • It prevented sending to 0x0
  • It blocked transfers of 0 value
  • It didn't return true or false for success or fail

The main cause for concern with this improper implementation is that if it is used with a smart contract that expects an ERC-20 token, it will behave in unexpected ways. It could even get locked in the contract forever.

Although standards aren't always perfect, and may someday become antiquated, they foster proper expectations to provide for secure smart contracts.

Suggested by: RobertMCForster

Sources

Assert Violation

In Solidity 0.4.10, the following functions were created: assert(), require(), and revert(). Here we'll discuss the assert function and how to use it.

Formally said, the assert() function is meant to assert invariants; informally said, assert() is an overly assertive bodyguard that protects your contract, but steals your gas in the process. Properly functioning contracts should never reach a failing assert statement. If you've reached a failing assert statement, you've either improperly used assert(), or there is a bug in your contract that puts it in an invalid state.

If the condition checked in the assert() is not actually an invariant, it's suggested that you replace it with a require() statement.

Sources

Requirement Violation

The require() method is meant to validate conditions, such as inputs or contract state variables, or to validate return values from external contract calls. For validating external calls, inputs can be provided by callers, or they can be returned by callees. In the case that an input violation has occurred by the return value of a callee, likely one of two things has gone wrong:

  • There is a bug in the contract that provided the input.
  • The requirement condition is too strong.

To solve this issue, first consider whether the requirement condition is too strong. If necessary, weaken it to allow any valid external input. If the problem isn't the requirement condition, there must be a bug in the contract providing external input. Ensure that this contract is not providing invalid inputs.

Sources