Default Visibility

Overview

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

What is the Default Visibility vulnerability?

In Solidity, visibility can be specified for state variables and functions on smart contracts. If visibility is not specified for a state variable, it is internal by default in modern versions of Solidity, meaning that that contract state can be read and modified by the contract and any that inherit from it. If data should not be modified by inheriting contracts, it should be marked as private. Failure to explicitly mark state variable visibility in particular may lead to situations where data is accessible or modifiable in unintended contexts. In general, explicitly marking the visibility is a good way to express developer intent and can help avoid inadvertent pitfalls.

Further reading: Solidity Documentation: Visibility and Getters

Technical example of vulnerable code

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

  contract AccessControl {
      // Mapping of address to bool indicating whether the address has admin access
      // By not specifying visibility, `isAdmin` is `internal` by default.
      mapping(address => bool) isAdmin;

      event AdminRightsGranted(address indexed to);
      event AdminRightsRevoked(address indexed from);

      constructor() {
          // Grant admin rights to the contract deployer
          isAdmin[msg.sender] = true;
      }

      // Modifier to restrict function access to only admins
      modifier onlyAdmin() {
          require(isAdmin[msg.sender], "Not an admin");
          _;
      }

      // Function to grant admin rights to an address
      function grantAdminRights(address _user) external onlyAdmin {
          isAdmin[_user] = true;
          emit AdminRightsGranted(_user);
      }

      // Function to revoke admin rights from an address
      function revokeAdminRights(address _user) external onlyAdmin {
          isAdmin[_user] = false;
          emit AdminRightsRevoked(_user);
      }

      // Check if an address is an admin
      function checkAdmin(address _user) external view returns (bool) {
          return isAdmin[_user];
      }
  }

  contract MaliciousInheritor is AccessControl {
      constructor() AccessControl() {}

      // Inadvertently or maliciously reset admin rights
      function resetAllAdmins() external {
          // A direct access like this could lead to unintended consequences
          // since `isAdmin` is `internal` and can be accessed here
          address[] memory admins = getAllAdmins();
          for (uint i = 0; i < admins.length; i++) {
              isAdmin[admins[i]] = false;
          }
      }

      // Placeholder for actual implementation
      function getAllAdmins() internal pure returns (address[] memory) {
          // This would be implemented to return the list of all admin addresses
          return new address[](0); // Simplified for example purposes
      }
  }

In the example above, contract AccessControl defines a state variable isAdmin, without specifying its visibility. This variable, which is meant to control admin rights for individual addresses, is thus internal by default, meaning contracts inheriting from AccessControl can modify this variable. In this case, the MaliciousInheritor contract exploits this access to enable resetting of all admin rights, which was not intended by the developer.

Technical example of how to fix the vulnerability

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

  contract AccessControlUpdated {
      // Mapping of address to bool indicating whether the address has admin access
      // By not specifying visibility, `isAdmin` is `internal` by default.
      mapping(address => bool) private isAdmin;

      event AdminRightsGranted(address indexed to);
      event AdminRightsRevoked(address indexed from);

      constructor() {
          // Grant admin rights to the contract deployer
          isAdmin[msg.sender] = true;
      }

      // Modifier to restrict function access to only admins
      modifier onlyAdmin() {
          require(isAdmin[msg.sender], "Not an admin");
          _;
      }

      // Function to grant admin rights to an address
      function grantAdminRights(address _user) external onlyAdmin {
          isAdmin[_user] = true;
          emit AdminRightsGranted(_user);
      }

      // Function to revoke admin rights from an address
      function revokeAdminRights(address _user) external onlyAdmin {
          isAdmin[_user] = false;
          emit AdminRightsRevoked(_user);
      }

      // Check if an address is an admin
      function checkAdmin(address _user) external view returns (bool) {
          return isAdmin[_user];
      }
  }

In the revised example above, contract AccessControlUpdated has marked the isAdmin state variable as private, meaning only the contract itself can modify it.