Directional Override Character

Overview

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

What is the Directional Override Character vulnerability?

Ethereum comments may contain Unicode characters, including directional override characters (for representing text left-to-right or right-to-left) that may be deliberately misleading, potentially encouraging a user of a smart contract function to supply parameters that will not do what they expect.

This vulnerability primarily relates to text written by others. Always be careful when reusing code that you did not write yourself, as it could potentially contain malicious properties as with this vulnerability.

Further reading: Wikipedia: Bidirectional Text - Overrides

Technical example of vulnerable code

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

  contract MaliciousBank
  {
      address payable o; // the contract owner
      mapping(address => uint256) balances;

      constructor() {
          o = msg.sender;
      }

      function deposit() public payable {
          uint256 currentBalance = balances[msg.sender];
          uint256 newBalance = currentBalance + msg.value;
          balances[msg.sender] = newBalance;
      }

      function withdraw() external returns(uint256)
      {
          uint256 balance = balances[msg.sender];
          address payable d = msg.sender;
          balances[msg.sender] = 0;
          _withdraw(/*owner‮/*noitanitsed*/d, o/*‭
                    /*value */, amount);
      }

      function _withdraw(address payable feeReceiver, address payable destination, uint256 value) internal
      {
          feeReceiver.transfer(1);
          destination.transfer(value - 1);
      }
  }

Depending on how your browser renders the above (i.e., whether or not it renders Unicode properly), you may notice nothing particularly out of the ordinary. However, a "plain text" rendering of the call to _withdraw() will reveal that the function is actually being called with the indicated parameters swapped, meaning that the user will not receive the full amount of ether that they expect:

  _withdraw(/*owner[U+202E]/*noitanitsed*/d, o/*[U+202D]
            /*value */, amount);

Given that the true order of the parameters in the call to _withdraw() is d, o, amount rather than the o, d, value ordering that the malicious string suggests, the user's balance will be sent to the owner of the contract rather than to themselves when they call withdraw().

Technical example of how to fix the vulnerability

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

  contract Bank
  {
      address payable o; // the contract owner
      mapping(address => uint256) balances;

      constructor() {
          o = msg.sender;
      }

      function deposit() public payable {
          uint256 currentBalance = balances[msg.sender];
          uint256 newBalance = currentBalance + msg.value;
          balances[msg.sender] = newBalance;
      }

      function withdraw() external returns(uint256)
      {
          uint256 balance = balances[msg.sender];
          address payable d = msg.sender;
          balances[msg.sender] = 0;
          _withdraw(o, d, amount);
      }

      function _withdraw(address payable feeReceiver, address payable destination, uint256 value) internal
      {
          feeReceiver.transfer(1);
          destination.transfer(value - 1);
      }
  }

In the updated code for contract Bank, the comments containing directional override characters have been stripped out of the contract, and the call to _withdraw() indicates the actual order of the arguments that will be passed to the function.