Introduction

One of the main pillars of secure applications is writing clean code. This is practiced through many principles and neglecting them often results in unnecessary attack vectors that lead to security issues. One of the baselines for clean code was formulated by Robert C. Martin in the SOLID principles. The first principle is the Single Responsibility Principle (SRP), or how “Uncle Bob” describes it:

A class should have only one reason to change.

In other words, a class should encapsulate a single, well-defined concern or functionality, promoting modular and maintainable code. A class is self-contained.

This principle translates 1-to-1 from object-oriented programming to smart contract development. For instance, the AccessManager contract released as part of the OpenZeppelin Library version 5.0 is a great example of a self-contained contract that enables authorization for multiple contracts at a time. On the contrary, the AccessControl contract is directly inherited by the respective contract such that changes to the authorization would require, for example, a token contract to change for a new mint role to be added. This concept is equally relevant on a function level: a function should be responsible for one action only. Overall, the SRP has the following advantages:

1. Conciseness: By writing concise code, unnecessary complexity is avoided which reduces the attack surface. This makes it easier to comprehend the purpose and functionality of the code.
2. Predictability: As every contract or function has only one task to address, their usage will be predictable. This makes it easier to assess possible outcomes of call-chains.
3. Maintainability: The code is easier to modify, update, and extend while reducing the impact on other parts of the smart contract. This also leads to easier troubleshooting and testing.

A Case Study

In the following, we will go over the importance of self-contained code for functions using the example of two critical audit findings. As introduced above, both issues originated from functions handling two tasks at once. This attack vector could be exploited in two ways: firstly, a front-running attack to install a backdoor, and secondly, destructing a contract to deny core functionalities.

The audited project is the MUD framework which implements the Store and World modules. The World is a Diamond Proxy-like design that functions as an entry point for systems (i.e., facets). These systems can implement arbitrary logic to interact with the World’s storage, while the Store module enables this storage to be handled in a table format. Hence, the World works as an abstraction layer on top of the Store. Users can register namespaces in this World in a permissionless way. This namespace ownership allows the creator to add and manipulate tables, add systems, transfer balances, and manage access control to this namespace or its resources.

Issue 1: Front-Running Backdoor

For the first issue, we want to look at the registerSystem function. This piece of code is responsible for registering the system so that it can be addressed through the World and has access to the namespace’s tables and systems. However, the registerSystem function is not entirely self-contained. There is a check present in the if-else conditions whereby a non-existent namespace will be automatically registered while an access control check is performed for the existing namespace. Although this is a better user experience, it also allows the following attack to happen.

Due to the permissionless nature of the project, the first branch enables an attacker to front-run the victim’s transaction to call registerNamespace with the victim’s namespace. This will make the attacker the namespace owner. In the same attacker transaction, further access to the namespace is granted to more attacker accounts. Afterwards, the ownership of the namespace is transferred to the victim. The victim’s transaction then executes the else branch while also passing the access control check. What the victim might not be aware of is that there is still an attacker with control over the tables and balance of their namespace.

function registerSystem(ResourceId systemId, 
WorldContextConsumer system, bool publicAccess) public 
virtual {
  // id, name, interface, and system existence checks...

  // If the namespace doesn't exist yet, register it
  ResourceId namespaceId = systemId.getNamespaceId();
  if (!ResourceIds._getExists(namespaceId)) {
    registerNamespace(namespaceId);
  } else {
    // otherwise require caller to own the namespace
    AccessControl.requireOwner(namespaceId, _msgSender());
  }

  // Check if a system already exists at this system ID
  address existingSystem = Systems._getSystem(systemId);

  // If there is an existing system with this system ID, 
remove it...
  // Otherwise, this is a new system, so register its 
resource ID...

  // Set new system in Systems, SystemRegistry, and 
ResourceAccess table...
}

function registerNamespace(ResourceId namespaceId) public 
virtual {
  // Require the provided namespace ID to have type 
RESOURCE_NAMESPACE
  if (namespaceId.getType() != RESOURCE_NAMESPACE) {
    revert World_InvalidResourceType(RESOURCE_NAMESPACE, 
namespaceId, namespaceId.toString());
  }

  // Require namespace to not exist yet
  if (ResourceIds._getExists(namespaceId)) {
    revert World_ResourceAlreadyExists(namespaceId, 
namespaceId.toString());
  }

  // Register namespace resource ID
  ResourceIds._setExists(namespaceId, true);

  // Register caller as the namespace owner
  NamespaceOwner._set(namespaceId, _msgSender());

  // Give caller access to the new namespace
  ResourceAccess._set(namespaceId, _msgSender(), true);
}

Abstracted registerSystem and registerNamespace functions of the WorldRegistrationSystem.

Issue 2: Destructed Contract

For the second issue, we will be looking at the registerTable function. Before we dive into this issue’s details, it is worth noting that this function is equally affected by the first issue. Once again, a check is performed on the existence of a namespace through the if-condition. In case the namespace of the registered table does not yet exist, an attacker is able to gain access with the same attack as described above.

To better understand this issue, we need to take a deeper look at the World’s design. Given that the World operates through namespaces and systems, this principle also affects the core functionality such as the discussed registrations, balance transfers, access management, and more. Thus, the core functionality is implemented through a core system which is registered under the root namespace. Since the core functions need to operate on the World’s storage, the core system needs to be delegatecalled from the World. Then, since WorldRegistrationSystem and StoreRegistrationSystem are different child contracts within the core system parent contract, a second delegatecall is performed to register a namespace from the registerTable function.

However, since the core system is a standalone contract, it has its own storage and can be called directly. This allows an attacker to register an arbitrary contract as the CORE_SYSTEM_ID system. As a next step, the attacker can invoke the registerTable function directly from the core system which will effectively delegatecall into the attacker-controlled contract, for example, to execute a selfdestruct. This eliminates all of the core functionality from the World.

function registerTable(
  ResourceId tableId,
  FieldLayout fieldLayout,
  Schema keySchema,
  Schema valueSchema,
  string[] calldata keyNames,
  string[] calldata fieldNames
) public virtual {
  // Table name check...

  // If the namespace doesn't exist yet, register it
  ResourceId namespaceId = tableId.getNamespaceId();
  if (!ResourceIds._getExists(namespaceId)) {
    // Since this is a root system, we're in the context of the 
World contract already,
    // so we can use delegatecall to register the namespace
    (address coreSystemAddress, ) = Systems._get(CORE_SYSTEM_ID);
    (bool success, bytes memory data) = coreSystemAddress.delegatecall(
      abi.encodeCall(WorldRegistrationSystem.registerNamespace, (namespaceId))
    );
    if (!success) revertWithBytes(data);
  } else {
    // otherwise require caller to own the namespace
    AccessControl.requireOwner(namespaceId, _msgSender());
  }

  // Register the table...
}

Abstracted registerTable function of the StoreRegistrationSystem.

Mitigation

Both attacks can be mitigated by applying the SRP. This is as simple as not invoking the namespace registration from the system and table registration and thereby forcing the user to do it explicitly. As a result, the functions will become more concise, maintainable, and predictable.

Throughout the audit, the Lattice team’s responsiveness and clarity in explanations were commendable. The code’s organization and documentation were also noteworthy. Despite the identified issues, the overall design and implementation of the system were evaluated positively.