Swapped Shift Parameters

Overview

• Severity: High
• Confidence: High
• Affected Versions: All

What is the Swapped Shift Parameters vulnerability?

The Solidity language allows for inlining of assembly instructions for developers who want to write low-level code. This can be useful for optimizing gas costs or for interacting with the Ethereum Virtual Machine (EVM) in ways that are not possible with Solidity alone. However, the inline assembly feature is also a source of many vulnerabilities, including the Swapped Shift Parameters vulnerability.

Shift assembly instructions take two parameters, a value to be shifted and the number of bits to shift by. The shl instruction shifts the value to the left, and the shr instruction shifts the value to the right. Both instructions take the number of bits to shift by as the first parameter and the value to be shifted as the second parameter. This means that if the parameters are swapped, unintended behavior is very likely to result.

Further reading: Yul EVM Dialect

Technical example of vulnerable code

  // SPDX-License-Identifier: Unlicense
  pragma solidity ^0.8.0;

  contract VulnerableTokenPresale {
      mapping(address => uint256) public reservations;
      bool saleEnded = false;
      address owner;

      constructor() {
          owner = msg.sender;
      }

      function reserve() public payable {
          uint256 weiSent = msg.value;
          uint256 reservationAmount;
          assembly {
              reservationAmount := shl(weiSent, 2);
          }
          reservations[msg.sender] = reservationAmount;
      }

      function endSale() public {
          require(owner == msg.sender);
          saleEnded = true;
      }

      function claimTokens() public {
          require(saleEnded);
          uint256 tokensToClaim = reservations[msg.sender];

          // functionality to mint tokens on an ERC-20 contract

      }
  }

In the example above, contract VulnerableTokenPresale intends to allow reservations for an ERC-20 token to be made in Ether. The reserve() function is intended to credit 4 tokens for each wei sent to the contract. However, the order of the parameters in the shl() instruction is reversed, meaning instead the value 2 will be shifted left by the amount of wei sent to the reserve() function, which can lead to two kinds of issues in this case.

The first issue relates to users who call the reserve() function without realizing that the instructions are reversed. If they send an amount of wei that will shift the value of 2 off the end of the integer size (e.g. 2 shifted left by 255 or more), they will be credited zero in their reservation, despite having sent ether to the contract. The second issue lies with users who realize the nature of the vulnerability, and who can subsequently reserve very large amounts of tokens for a very minimal amount of wei. Neither of these effects are intended, and both undermine the intended purpose of the contract.

Technical example of how to fix the vulnerability

  // SPDX-License-Identifier: Unlicense
    pragma solidity ^0.8.0;

    contract TokenPresale {
        mapping(address => uint256) public reservations;
        bool saleEnded = false;
        address owner;

        constructor() {
            owner = msg.sender;
        }

        function reserve() public payable {
            uint256 weiSent = msg.value;
            uint256 reservationAmount;
            assembly {
                reservationAmount := shl(2, weiSent);
            }
            reservations[msg.sender] = reservationAmount;
        }

        function endSale() public {
            require(owner == msg.sender);
            saleEnded = true;
        }

        function claimTokens() public {
            require(saleEnded);
            uint256 tokensToClaim = reservations[msg.sender];

            // functionality to mint tokens on an ERC-20 contract

        }
    }

In the corrected example, contract TokenPresale uses the correct order of the shl() parameters to achieve the intended calculation. Alternatives could involve using multiplication directly rather than shift instructions, and the resulting value could be checked for potential overflow before being credited to the user.