Delegatecall in Loop

Overview

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

What is the delegatecall in Loop vulnerability?

Addresses in Solidity may be targeted with low-level calls including delegatecall() that include a payload which may be used for function invocation if desired. If the the delegated call targets a payable function that uses msg.value, and the call is within a loop, the message value may be incorrectly used in a way that can lead to accounting errors.

Further reading: Solidity Documentation: Members of Address Types

Technical example of vulnerable code

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

  contract FundsManager {
      mapping (address => uint256) balances;

      function addBalance(address a) public payable {
          balances[a] += msg.value;
      }

      // Other functionality omitted for brevity
  }

  contract FundsDistributor {
      mapping (address => uint256) balances;

      event FailedSend(address indexed badRecipient);

      FundsManager fm;

      constructor(address _fundsManagerAddress) {
          fm = FundsManager(_fundsManagerAddress);
      }

      function addBalanceAcrossMany(address[] memory receivers) public payable {
          for (uint i = 0; i < receivers.length; i++) {
              (bool success, ) = address(fm).delegatecall(abi.encodeWithSignature("addBalance(address)", receivers[i]));
              if (!success) {
                  emit FailedSend(receivers[i]);
              }
          }
      }
  }

In the example above, contract FundsManager provides a function addBalance() that is designed to track ether funds received. Contract FundsDistributor calls this function remotely via delegatecall(), which will use the msg.value from the initial call repeatedly, leading to recording of balances that does not match the actual ether received.

Technical example of how to fix the vulnerability

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

  contract FundsManagerUpdated {
      mapping (address => uint256) balances;

      function addBalance(address a, uint256 amount) public {
          balances[a] += amount;
      }

      // Other functionality omitted for brevity
  }

  contract FundsDistributorUpdated {
      mapping (address => uint256) balances;

      event FailedSend(address indexed badRecipient);

      FundsManagerUpdated fm;

      constructor(address _fundsManagerAddress) {
          fm = FundsManagerUpdated(_fundsManagerAddress);
      }

      function addBalanceAcrossMany(address[] memory receivers) public payable {
          uint256 splitValue = msg.value / receivers.length;

          for (uint i = 0; i < receivers.length; i++) {
              (bool success, ) = address(fm).delegatecall(abi.encodeWithSignature("addBalance(address,uint256)", receivers[i], splitValue));
              if (!success) {
                  emit FailedSend(receivers[i]);
              }
          }
      }
  }

In the revised example above, contract FundsManagerUpdated now performs only accounting and not the actual handling of ether, using information calculated by the addBalanceAcrossMany() function in the FundsDistributorUpdated contract.