Table of Contents
- Table of Contents
- System Overview
- Security Model and Trust Assumptions
- Critical Severity
- Medium Severity
- Low Severity
- Missing Table Registration
- Off-Chain Indexers Can Lose Track of On-Chain State
- Namespace Balance Transfer Value Can Be Lost
- Delegation Can Be Misconfigured
- Deployment Edge Case
- Incorrect ERC-165 Interface
- Incomplete Table Validation
- Incomplete Module Access Control
- Incomplete Resource ID Validations
- Inexplicit Revert
- World Resource ID ROOT String Has Unexpected Length
- Override Removes Supported Interface
- Notes & Additional Information
- Encapsulate Functionality Recommendation
- Unintuitive Order of Function Arguments
- Naming Suggestions
- Unused Functions and Variables
- Unused Imports
- Duplicate Imports
- Visibility Not Explicitly Declared
- Code Simplification Suggestions
- Magic Number
- Typographical Errors
- Unnecessary Use of Generic Function
- From 2023-10-03
- To 2023-11-14
- Total Issues
- 32 (32 resolved)
- Critical Severity Issues
- 2 (2 resolved)
- High Severity Issues
- 0 (0 resolved)
- Medium Severity Issues
- 6 (6 resolved)
- Low Severity Issues
- 12 (12 resolved)
- Notes & Additional Information
- 11 (11 resolved)
- Client Reported Issues
- 1 (1 resolved)
In scope were the following contracts:
│ └── src
│ ├── Bytes.sol
│ ├── FieldLayout.sol
│ ├── Hook.sol
│ ├── Memory.sol
│ ├── PackedCounter.sol
│ ├── ResourceId.sol
│ ├── Schema.sol
│ ├── Slice.sol
│ ├── Storage.sol
│ ├── StoreCore.sol
│ ├── StoreData.sol
│ ├── StoreHook.sol
│ ├── StoreRead.sol
│ ├── StoreSwitch.sol
│ ├── constants.sol
│ ├── leftMask.sol
│ ├── storeHookTypes.sol
│ ├── storeResourceTypes.sol
│ └── tightcoder
│ └── TightCoder.sol
│ └── core
│ ├── CoreModule.sol
│ ├── constants.sol
│ └── implementations
│ ├── AccessManagementSystem.sol
│ ├── BalanceTransferSystem.sol
│ ├── BatchCallSystem.sol
│ ├── ModuleInstallationSystem.sol
│ ├── StoreRegistrationSystem.sol
│ └── WorldRegistrationSystem.sol
The MUD system provides a cohesive range of standard and extensible functionality to quickly develop blockchain applications.
The central component of the MUD system is the
Store, which is a contract that behaves like a database. All persistent storage is presented as a set of tables, and the
Store contract takes care of mapping this structure to the linear EVM storage layout. Importantly, the structure is not fixed at deployment time, which means developers can easily add new tables or any other functionality (described below) as their application evolves.
In addition, the
Store contract allows multiple applications to provide different overlapping views of the same underlying data. This is achieved by standardizing the events that describe all state changes, so off-chain tooling can represent the storage in an application-agnostic way. In fact, some tables can be designated as off-chain tables, where they will emit the standard event stream on storage changes but the actual data will not be saved on-chain and must be reconstructed from these events.
Tables can also include hooks. These are arbitrary code snippets that can be executed whenever a record is added, removed or modified. This would typically be used for data validation, or to ensure consistency within a database record.
Store contract provides the basic database functionality, it will typically be extended to provide higher-level abstractions for users and developers. One such extension is the
World contract (in conjunction with the
CoreSystem contract) which provides generic and flexible mechanisms for many standard features.
For example, the
World contract registers tables in the
Store contract to track access control relationships for all the other tables. Specifically, the database is segmented into different namespaces each of which has an owner address. The namespace owner can create new tables and manage which addresses can modify which tables in the namespace.
In addition, the namespace owner can register systems which are arbitrary contracts that are granted access to the namespace. These would typically be used for application-specific logic, to modify multiple records and tables in the same transaction, and to do so in a consistent way. They can even register system hooks (arbitrary code snippets) to run before or after the system is invoked. Moreover, there are some convenience functions which can be used to invoke systems more easily or to make multiple calls in a single transaction. The namespace owner also controls which addresses can invoke which systems.
World contract also provides a delegation mechanism whereby any address can allow other addresses to perform actions on its behalf. If desired, the delegation can be managed by an arbitrary contract, so approval can be granted or rejected based on the details of the action and the particular delegatee. The namespace owner can also provide a fallback namespace delegation contract that manages all delegations for systems within the namespace.
The ETH held by the
World contract is partitioned among the namespaces. This provides the systems in the namespace with a convenient way to share the same balance. Naturally, each individual system contract can also maintain a private ETH balance if that is desired.
Lastly, there is a root namespace which can be used to register privileged systems with complete control over the storage. In this way, the behavior of any deployed
World contract is upgradeable and customizable.
Security Model and Trust Assumptions
The codebase is intended to be deployed by application developers. Depending on the use case, an application might require its own
World or it could use an existing
World instance, possibly with its own namespace for encapsulation. In either case, all
Worlds are independent and the Lattice team does not retain any privileged access over them.
In the standard use case, the
World deployer will initialize the system to become the owner of the root namespace, the world namespace, and the store namespace. They will then create the tables and records required to implement the features described above.
Anyone can register a new namespace and become its owner. This will let them manage the access control requirements (including transferring and renouncing ownership) and create new tables, systems, and hooks within the namespace.
However, it is worth noting that the root namespace owner can install new privileged systems or replace the
CoreSystem functionality which implicitly gives them control over all the resources. Therefore, unless they renounce their power, they should be trusted by all the
Possible Data Corruption
In the interest of efficiency, the system intentionally bypasses many data integrity checks. For example, users can set a table field to a value that is larger than what the type supports, which would overflow into subsequent fields, possibly beyond the allocated space. Similarly, users do not need to follow the expected key format when referencing records which could lead to them storing values at "invalid" pseudorandom locations. Users who have been granted write access to a table are implicitly trusted not to corrupt the data in this way.
It is worth noting that the system already relies on the assumption that pseudorandom locations (for all records in all tables in all namespaces) are sufficiently separated to prevent data collisions between different records. The ability for users to overwrite locations near their legitimate records, or at some other pseudorandom locations, does not undermine the basic assumption. Although users can corrupt their own tables, it should still be impossible to manipulate records in other tables and namespaces.
Namespace Access Can Be Backdoored
WorldRegistrationSystem contract takes care of registering namespaces, among other things. A namespace sets the context for a user to deploy systems and register tables. Ownership of or access to this namespace allows for the performance of critical operations such as setting
Store values or transferring balance. Access to a namespace also gives access to all resource-specific systems or tables.
However, there is a problem when it comes to the arbitrary options of registering namespaces. For instance, consider the following attack. It assumes that the victim initially registers something for a new namespace (so the namespace does not yet exist):
- The user attempts to register a system or a table, which would implicitly register the namespace.
- The attacker front-runs the victim to:
- The victim proceeds with (1). The call succeeds because while the namespace already exists, the victim is the legitimate owner.
- The attacker has full control over anything that requires access such as balance transfer or
Consider making the design more self-contained by removing the automatic namespace registration from the aforementioned functions. Instead, make it mandatory for a user to register the namespace beforehand such that a front-run would make the user's call fail due to the existence check.
Update: Resolved in pull request #2007.
Core System Can Be Disabled
As a pure implementation contract, there are no access control protections when its functions are invoked directly (bypassing the
World). In particular, an attacker can register a system for the
CORE_SYSTEM_ID and pass in any
_msgSender value in the calldata to bypass the access control check. This is not an attack in itself because the contract storage of the
CoreSystem contract is not relevant to the
World contract. However, when the attacker invokes the
registerTable function directly on the
CoreSystem, it will make a
delegatecall to the attacker-controlled core system, which could execute the
selfdestruct opcode. This will remove the
CoreSystem, effectively disabling it for the
Consider removing the namespace registration branch so that the
CoreSystem will not execute a
delegatecall. Alternatively (or in addition), consider protecting all
CoreSystem functions with an
Incorrect Hook Parameter
Any storage hook that implements the
onBeforeSpliceDynamicData function should receive the state of the record before the update. However, it will receive the updated encoded lengths instead, which may cause the hook to respond incorrectly.
Consider passing the original encoded lengths.
Update: Resolved in pull request #2020.
requireInterface Is Incorrectly Specified
requireInterface function is incorrectly specified in two ways.
Firstly, it uses a
try-catch block to revert with a meaningful error if the contract does not implement the expected interface. However, this will only catch errors that occur in the context of the target contract. If the target contract has no code, or returns a value that cannot be decoded as a boolean, the error will occur in the caller's context and will revert outside the
Secondly, it discards some requirements of the EIP specification. To be compliant, it should confirm that the
supportsInterface function returns
false for the invalid interface
0xffffffff, and that it does not consume more than 30000 gas. This ensures that unrelated contracts that happen to have a fallback function that returns at least 32 bytes will not mistakenly pass the validation.
Consider using the OpenZeppelin
ERC165Checker contract, or correcting these deviations.
Update: Resolved in pull request #2016.
Sliced Bytes Are Cut Off
Bytes library, the
slice4 function intends to return a
bytes4 value from a
bytes32 value at a given index. While the output is correctly returned as a
bytes4 value, the actual output variable is declared as a
bytes2 value. This leads to only two correct bytes being returned from the index position followed by two zero bytes. The wrong data could escalate into more severe security issues depending on the context.
Consider correcting the output variable type to
Update: Resolved in pull request #2031.
Memory Corruption on Load From Storage
Storage library, the
load function can load data from a storage location into a memory pointer location. The loaded data can further be specified by an offset in the storage and a length to enable loading data that spans over multiple slots.
However, there is an edge case in the parameter input set that causes memory corruption. This is when data is loaded from an offset position, but the length is less than the remainder of the slot:
storage slot: [ 0 ---------- 10 ---------- 20 ----------- 31 ]
<-- length -->
<------- word remainder ------->
The problem occurs because the bitmask is based on the word remainder instead of accounting for the length. This leads to extra bytes being written into memory (e.g., bytes 20 to 31 in the example above).
There are no significant consequences in the current version of the codebase. This is because all memory loads either use a zero offset or invoke the three-parameter version which reserves sufficient space to cover and ignore the unwanted bytes. Nevertheless, the library is intended to support external codebases and the inconsistency may lead to arbitrarily severe memory corruption.
Consider respecting the length when constructing the bitmask as seen in the
Update: Resolved in pull request #1978.
registerFunctionSelector Can Be Front-Run and DoS'ed
registerFunctionSelector function is used to register system function selectors for the
World context. This unique
bytes4 worldFunctionSelector is based on the user-provided
systemFunctionSignature. In the
fallback function of the
World contract, the
worldFunctionSelector is taken as a look-up table value to forward the call to the right system and function. This mechanism exists in parallel to the
callFrom functions that allow the user to specify the system to address through a function parameter.
The problem is that function selectors are only four bytes, so collisions (technically, second-preimages) can be realistically generated. This is particularly true if the
worldFunctionSelector is known ahead of time (e.g., in the context of DAO voting). If a
worldFunctionSelector is already taken, subsequent registrations will revert. This allows a malicious actor to brute-force a
worldFunctionSelector that is identical to that of the victim and front-run the transaction. Hence, the legitimate victim transaction would fail.
worldFunctionSignature is constructed by concatenating the namespace, name, and system function signature with underscores. If the function name contains underscores itself, a selector collision can be trivially constructed. For example, the
MyNS_MySystem_do_things() can be broken down into:
The impact of this attack is Denial of Service with the intention of griefing. The victim would have to re-write their code, redeploy it, and try to register it again. Alternatively, they will be unable to use this selector mapping feature.
Another collision that may occur is between registered
worldFunctionSignatures and any of the external/public functions in the
World contract. Since the external/public function is prioritized over the fallback function, the intended system call could lead to two problems:
- The call reverts due to a parameter decoding mismatch or access control check.
- The call succeeds and performs an unexpected action.
Consider changing the world function selector delimiter from an underscore to a character that is invalid for function names (e.g., a colon). Also, consider registering the
World's functions so that they can't be registered for the fallback function mechanism. However, the front-running issue will persist without a larger redesign. Hence, consider reducing the attack surface and code complexity by removing this feature entirely, or possibly restricting it to the root namespace and enforcing the usage of the
callFrom functions that address systems specifically.
The fixes address the potential conflicts between function selectors of different namespaces. It does not address the possibility of front-running namespace/function selector registration which we've decided to punt on for now and later address with an optional module. This module will allow "committing" to a hashed namespace and then "revealing/registering" the namespace in a second step. Since C-01 is fixed, this is not a security issue anymore but just a potential griefing vector.
We have also decided to punt on addressing the possibility of brute-force calculating a conflicting function selector - since the value space is only 4 bytes it seems impossible to prevent this - but the feature is too useful to remove it altogether. The only issue this could lead to is a griefing attack if a function selector is known long enough before it is registered. This seems like a relatively small issue compared to the value custom function selectors provide in the vast majority of situations.
Throughout the codebase, there are multiple instances of misleading documentation:
- In the
loadFieldfunction of the
Storagelibrary, the documentation suggests that the bytes beyond the
lengthparameter are zero. However, considering a storage slot as
[ aa..aa bb..bb cc..cc ]and
bb..bbbeing the desired field, then despite its respective
length, the result would return
cc..ccas part of the bytes response. As such, the documentation is misleading and can cause consecutive memory corruption. Consider cleaning up the memory space by applying a bitmask or updating the documentation.
- In the
zerofunction of the
lengthparameter's documentation suggests it should be specified in words. However, in the code, the value is expected to be in bytes. This can lead to a shortcoming in overwrites to zero and therefore in preserving old data, which can lead to additional security issues. Furthermore, the length is rounded up to the nearest 32 bytes multiple which is not clear either. Consider clarifying the
- The comments in the
Storage.loadfunction refer to the masking as "middle part" and "surrounding parts" whereas it is actually the left and right parts.
- The code comments for all the
packfunctions of the
PackedCounterLiblibrary read "Packs a single value into a PackedCounter", but most of them pack multiple values.
- The documentation of the
leftPaddingBitsparameter in the
TightCoder.encodefunction is ambiguous. It says, "The number of bits to pad on the left for each element", but the code actually shifts the values by that count to the left. For instance, this means when encoding a
leftPaddingBitscount would be 136 as seen in the
Consider correcting the documentation to align with the code's behavior. This will help improve the clarity and readability of the codebase.
Update: Resolved in pull request #2100.
Missing Table Registration
This issue was independently identified and fixed by the Lattice team during the audit.
The Core module registers the tables it uses, but it excludes the
FunctionSignatures table. This prevents the indexers from decoding the
FunctionSignatures events. Consider registering this table as well.
Off-Chain Indexers Can Lose Track of On-Chain State
StoreCore contract allows for the manipulation of static and dynamic data in the tables. This includes setting a record of table elements, splicing static and dynamic data, and deleting a record. Hooks before and after the respective action enable reactivity to these storage changes. Whenever one of these functions is called, an event is emitted to notify off-chain indexers about the latest storage changes. It is expected that the indexers can recreate the on-chain storage from these events.
A problem can arise due to the order of events and calls within any of the aforementioned functions which can cause an indexer to perceive a different state outcome than what happened on-chain. The flow for those functions is generally the following:
- Emit an event.
- Call the "before" hooks.
- Do the storage change.
- Call the "after" hooks.
However, a malicious or mistaken namespace owner can set up a hook that can reenter any of those functions to change the flow (simplified) to the following:
- Emit event
- Reenter through the "before" hook.
- Emit event
- Do storage change
- Emit event
- Do storage change
As such, although event
B was emitted last, storage change
A is in fact true. For instance, this could cause major confusion when the same record is first deleted and then created which would be perceived in a reversed order off-chain.
Consider placing the event emissions together with the actual state change to prevent this type of confusion for off-chain indexers.
Update: Resolved in pull request #2017.
Namespace Balance Transfer Value Can Be Lost
BalanceTransferSystem is used to send a namespace's accounted ETH value to either another namespace or to an address. However, balances can be sent to a non-existent namespace. This could occur due to human error and would likely lead to a loss of funds. Although the non-existent namespace could be registered afterwards, any other user could front-run this registration to get control over the funds.
Consider checking whether the recipient namespace exists before proceeding with the transaction.
Update: Resolved in pull request #2095.
Delegation Can Be Misconfigured
When setting up delegation control with a system, it is possible to register an invalid delegation control without it being immediately noticeable. This would be the case when
initCallData is not required (has length zero), which causes the registration function to skip the interface conformance check of the delegation control system. An attacker can take advantage of this mistake, if the delegation is not account-specific, by registering this non-existent system ID to thereby gain control over the system.
Consider performing the interface checks on the delegation control system in all cases, whether or not
initCallData is required. It should be noted that this would prevent clearing the delegation (i.e., resetting the
delegationControlId to zero). Thus, consider introducing a new function to unregister delegations.
Update: Resolved in pull request #2096.
Deployment Edge Case
initialize function sets up the
CoreSystem functionality. To make it only callable once, it checks if there is no existing
"core" module installed.
However, the core module is installed under the name that the module responds with. If it responds with a different name,
initialize will still be callable. This should not happen if the
World creator is honest. A malicious owner can utilize this function as a backdoor through the delegate call. As the
initialize function's access is bound to the creator role, this attack vector is active despite potentially revoking any root namespace ownership. Also, it allows overwriting any other record in the table of installed modules.
It is worth noting that the initial validation reads a record from the
InstalledModules table before it is registered, and the subsequent update assumes the core module registered the table. This still works whether or not the core module actually registers the
InstalledModules table because both operations directly access the contract storage location where the relevant table record would be stored. Nevertheless, it undermines the expected table abstraction.
Consider explicitly installing the core module under the
CORE_MODULE_NAME to ensure the
initialize function is single-use. Alternatively, to avoid accessing an unregistered table, consider introducing a new initialization flag.
Update: Resolved in pull request #2170. There is a new
CoreModuleAddress table to save the address.
Incorrect ERC-165 Interface
The following interface IDs include the
This is inconsistent with the EIP (as can be seen by the Simpson example). Consider excluding the
ERC165_INTERFACE_ID constant from the custom interface IDs.
Update: Resolved in pull request #2014.
Incomplete Table Validation
There are several consistency checks when registering a new table, but they are incomplete. In particular:
- The field layout validation does not confirm whether the total static field length is the sum of all the individual static field lengths, or that the unused length fields (if the maximum number of fields is not reached) are all zero.
- The schema validations for the key and value schemas do not confirm whether the total static field length is consistent with the static schema types.
- The field layout and value schema are not confirmed to be consistent with each other, except for having the same total number of fields. This means that the number of static fields is not necessarily the same, and the length of a static field in the field layout does not necessarily match the type of the static field in the value schema.
Inconsistent table specifications may interfere with saving, retrieving and interpreting the database records. Consider including these additional validations.
Update: Resolved in pull request #2046.
Incomplete Module Access Control
ModuleInstallationSystem allows anyone to install any module. Since this makes an external call to the module, it does not provide any additional functionality. Instead, it is simply a convenient way to execute several related operations atomically. However, it can overwrite any record in the
InstalledModules table, which doesn't have any subsequent effects in the current codebase.
Consider whether the
InstalledModules table should use the module address (along with the arguments hash) instead of the module name as the record key. This is so that it identifies the action that was taken more directly and is more resistant to being overwritten. Alternatively, consider whether the
InstalledModules table should be removed entirely.
Update: Resolved in pull request #2168.
Incomplete Resource ID Validations
Resource IDs in the
World are expected to encode three components (the type, namespace and name) that are used to ensure consistency between different table records. However, there are several instances of incomplete consistency checks:
- It is possible to call
transferOwnershipon any resource ID, provided the caller owns the corresponding namespace. This includes a table, system, an unknown resource ID, or an incorrectly specified resource ID.
- It is possible to call
toNamespaceIdthat has a non-zero "name" component. This will effectively transfer the funds to a new account in the destination namespace, which needs to be spent individually (i.e., it cannot be consumed by systems within the namespace unless they are explicitly designed to handle it).
- It is possible to call
registerFunctionSelectorwith any resource ID, provided the caller owns the corresponding namespace. In practice, if it does not correspond to a valid system, it will be unusable.
- It is possible to
grantAccessto an incorrectly specified resource ID, or a resource that does not exist.
- It is possible to call
namespaceIdthat has a non-zero "name" component.
- It is possible to call
registerSystemHookwith any resource ID, provided the caller owns the corresponding namespace.
- It is possible to call
registerStoreHookwith a non-existent
tableId, provided the caller owns the corresponding namespace.
In the interest of predictability and limiting the attack surface, consider validating all three components of user-provided resource IDs and validating the existence of a resource wherever relevant throughout the codebase.
getDynamicFieldSlice function of the
StoreCore library, a bytes range specified by
end can be read from a dynamic field.
While the range is checked to be within the field's length, the order of
end is not checked. If
start were to be greater than
end, the subtraction for the length would underflow and the transaction would revert implicitly without providing any contextual information.
Consider adding a range check to fail more explicitly.
Update: Resolved in pull request #2034.
World Resource ID ROOT String Has Unexpected Length
WorldResourceIdInstance library has helper functions to get information from an encoded
World resource ID such as type, namespace and name. These fields can also be returned as a string using the
Consider changing the constant's type from
Update: Resolved in pull request #1976.
Override Removes Supported Interface
Module contract overrides the
supportsInterface function and no longer supports the
WORLD_CONTEXT_CONSUMER_INTERFACE_ID. This is inconsistent with the equivalent
DelegationControl override, which retains support for the
Consider ensuring ERC-165 support for all implemented interfaces.
Update: Resolved in pull request #2032.
Notes & Additional Information
Encapsulate Functionality Recommendation
Several functions in the codebase have side effects or handle multiple use cases:
AccessManagementSystemdoes not provide a mechanism to renounce the ownership of a namespace, so users will need to transfer ownership to an uncontrolled address (like the zero address). However, this will create an unnecessary record in the
registerTablefunction might register a namespace as well.
registerSystemfunction might register a namespace or delete a system. In fact, this is the only way to delete a system using the
registerSystemfunction also accepts an existing system and system ID pair, which will delete the system records only to recreate them again, possibly changing the
publicAccessflag in the process.
- Individual delegations must be removed by overwriting them using the regular registration function, which still leaves a stray record in the
- Namespace delegations cannot be set to zero so they must be removed by creating a new delegation control that rejects all calls.
The stray records could potentially be addressed by directly updating the tables, but this is fragile and undermines the abstractions provided by the
World contract. Similarly, functions with side effects or multiple use cases can be fragile, and the most significant issues in this report are a result of imprecise handling at the boundaries of different functionality.
To reduce the attack surface and increase predictability, consider limiting each function to a single use case with a linear code path where possible.
Update: Resolved in pull request #2157.
Unintuitive Order of Function Arguments
Storage library contains functions to
load data according to the storage pointer, offset, length, and memory pointer. However, both functions take these arguments in different orders which can be non-intuitive for a developer.
Consider aligning the order of function parameters when the set of arguments overlaps.
Update: Resolved in pull request #2033.
Throughout the codebase, there are code element names that are ambiguous:
_staticFieldsparameter of the
encodefunction could be called
requireNoCallbackmodifier could be called
byteCodeparameter in the
deployfunction could be called
creationCodeas referred to in the documentation.
tableWithHooksparameter of the
filterListByAddressfunction could be called
elementWithHooksto account for the possibility that it refers to a system.
Update: Resolved in pull request #2091.
Unused Functions and Variables
Throughout the codebase there are multiple instances of unused functions and variables:
- In the
Byteslibrary, the functions
setBytes7, as well as some of the
Consider removing this code to improve the overall clarity and readability of the codebase.
Update: Resolved in pull request #2090.
Throughout the codebase, the following imports are unused and could be removed:
Consider removing unused imports to improve the overall clarity and readability of the codebase.
Update: Resolved in pull request #2028.
Consider removing duplicate imports to improve the readability of the codebase.
Update: Resolved in pull request #2024.
Visibility Not Explicitly Declared
There are some instances of missing explicitly declared visibility:
Consider always explicitly declaring the visibility of variables and functions, even when the default visibility matches the intended visibility.
Update: Resolved in pull request #2029.
Code Simplification Suggestions
Throughout the codebase, there are multiple instances where the code could be simplified:
- The schema type validation loop in the
Schemalibrary can be simplified. Consider looping over the
_numStaticFieldsschema types first to ensure they all have a non-zero static byte length. Then, loop over the
_numDynamicFieldsschema types to ensure they have a zero static byte length. This way, it is not necessary to count the fields (and validate the count later) or branch inside the loop.
Memory.copyfunction is redundant to the identity precompile. Consider replacing the function's logic with a call to the precompile for simplicity and efficiency.
leftMaskfunction is constructed by first creating a right mask (of size
32 - byteLength) and then inverting it. However, the actual usage involves using both left and right masks [1, 2, 3, 4, 5]. Hence, the left mask is inverted back to a right mask. Consider returning a right mask in the first place.
- In contrast to, for instance,
ResourceIddoes not have a
usingstatement in the
ResourceId.solfile. Instead, it is duplicated several times throughout the codebase.
- There is an inconsistency for the integer base in assembly blocks. While most of the codebase uses hexadecimal numbers, there are instances [1, 2, 3, 4, 5] where decimal numbers are used.
- For EIP-165 support, the codebase maintains constants [1, 2, 3, 4, 5, 6, 7] that reflect the supported interface IDs which are checked against in the
supportsInterfacefunctions. Instead, consider making use of the interface ID provided by the interface type like
FieldLayout.numFieldsfunction could make use of the
numDynamicFieldsfunctions instead of reimplementing them.
- In the
Schema.solfiles, there is an inconsistency when it comes to the leading underscores for stack variables.
- There is an inconsistency of relying on default initialization to zero [1, 2] versus explicitly initializing to zero .
- In the
StoreCore.getFieldLayoutfunction, instead of getting the field layout through
Storage.loadField, consider using
Tables._getFieldLayout, similar to how it is done for the key schema and value schema.
WorldContextConsumer._world, the call to
WorldContextConsumerLib._worldfor consistency with the rest of the contract.
coreModulestate variable of the
WorldFactoryis set in the constructor and never changed. Consider making it
WorldRegistrationSystem.registerSystemfunction accepts a
WorldContextConsumertype argument, whereas it should accept a
slicefunctions in the
Byteslibrary can be simplified by declaring the
outputvariable as a named return to save on the extra declaration and return in the function body.
- The validation condition in the
getSubslicefunction could be replaced with
start > end || end > data.length.
extcodesizeopcode is not necessary to check whether the
Create2.deploycall was successful since the address will also be zero in that case.
Consider applying the code changes as outlined above to improve the overall clarity and readability of the codebase.
Update: Resolved in pull request #2140. The Lattice Labs team stated:
We chose not to implement three suggestions due to gas use and readability concerns.
Update: Resolved in pull request #2015.
The following typographical errors were identified in the codebase:
- "Since the they're max 32 bytes" has an extra "the"
- "@param e The length of the fourth dynamic field's data" should say "[...] fifth dynamic field's data"
- The constant
DYNMAIC_DATA_SLOTshould be called
- "A namespace can includes tables and systems" should say "include"
- "Require the balance balance to be greater or equal to the amount to transfer" says "balance" twice
Consider fixing all instances of typographical errors throughout the codebase.
Update: Resolved in pull request #2023.
Unnecessary Use of Generic Function
Update: Resolved in pull request #2022.
Store Namespace Unregistered
CoreModule is installed, the root namespace and world namespace are both registered with the creator as the owner. However, the store namespace is not registered. If another user registers it later, they would control the store tables (i.e., Tables, StoreHooks and ResourceIds), which essentially gives them control over the entire
The MUD system is a comprehensive framework for rapidly developing blockchain applications. It revolves around a central component called the
Store, which acts as a flexible database contract allowing developers to dynamically add tables and functionalities. The
World extends this functionality, enabling higher-level abstractions, access control, system registration, and delegation mechanisms, providing a customizable and upgradable structure for blockchain applications.
Throughout the audit period, the Lattice team was very responsive and provided us with useful explanations and clarifications. We also appreciated the code's organization and documentation. As noted in the report, our main suggestions involved limiting the number of branching code paths to limit the attack surface. The Lattice team implemented all suggested improvements. Overall, we believe the system is well-designed and implemented.