We performed a diff audit of pull request #31 of the dojoengine/dojo-core repository at commit a2ee681 against commit e1a349b of the dojoengine/dojo repository.
In scope were the following files:
crates/contracts/src
├── contract
│ ├── components
│ │ ├── upgradeable.cairo
│ │ └── world_provider.cairo
│ ├── contract.cairo
│ └── interface.cairo
├── event
│ ├── event.cairo
│ ├── interface.cairo
│ └── storage.cairo
├── lib.cairo
├── model
│ ├── component.cairo
│ ├── definition.cairo
│ ├── entity.cairo
│ ├── interface.cairo
│ ├── members.cairo
│ ├── metadata.cairo
│ ├── model.cairo
│ ├── model_value.cairo
│ └── storage.cairo
├── utils
│ ├── descriptor.cairo
│ ├── hash.cairo
│ └── naming.cairo
└── world
├── errors.cairo
├── iworld.cairo
├── storage.cairo
└── world_contract.cairo
We have already described the system in our previous security review report and, therefore, will only detail the major system changes in this section. The main change introduced by the subject pull request is to pass the namespace as a parameter when registering or updating events, models, or systems, instead of requesting it from the corresponding contract. This simplifies the security analysis by removing the possibility for a particular contract to change its reported namespace whenever it is queried. It also removes the need for contracts to know their own namespace, namespace hash, tag, and selector.
The other change is to provide a more structured set of traits to define the relationship between models (and events) and their associated storage implementations. This allows developers to associate a particular namespace with a model (or event) so that it can be implicitly included in the selector, and it also allows developers to change the namespace as desired. The existing storage architecture is now just one possible implementation of the storage interface.
Lastly, we will note that the codebase has been updated since our last audit to include events as first-class resources. They have been structured as models but can be used when on-chain storage is not required. This provides a mechanism for developers to emit detailed custom events.
Clear inline documentation is fundamental for outlining the intentions of the code. Mismatches between the inline documentation and the implementation can lead to serious misconceptions about how the system is expected to behave. The inline documentation for the Resource enum is missing a description of the Event option.
Consider fixing this error to avoid confusion for developers, users, and auditors alike.
Update: Resolved in pull request #2640 of the dojo repository, and pull request #34 of the dojo-core repository.
The IWorldProvider trait has renamed the world function to world_dispatcher to be more precise. For consistency, renaming the world field in WorldStorage would help prevent any confusion in understanding the code.
Consider applying the above-given naming suggestion to improve the consistency and readability of the codebase.
Update: Resolved in pull request #32. However, we only reviewed the proposed fixes pertaining to the issues within the pull request. All other major code changes were not reviewed.
During development, having well-described TODO comments will facilitate the process of tracking and solving them. Without this information these comments may age and important information for the security of the system could be forgotten by the time it is released to production. Throughout the codebase, multiple instances of unaddressed TODO comments were identified:
Consider removing all instances of TODO comments and instead tracking them in the issues backlog. Alternatively, consider linking each inline TODO to the corresponding issues backlog entry.
Update: Resolved in pull request #33. However, we only reviewed the proposed fixes pertaining to the issues within the pull request. All other major code changes were not reviewed.
In the naming.cairo contract, one instance of code simplification was identified. The condition for this if statement could be written as break (i > 0).
Consider making the above change to simplify the code and make it easier to understand for future reviewers.
Update: Resolved in pull request #35 of the dojo-core repository, and pull request #2639 of the dojo repository.
Update: Resolved in pull request #2645 of the dojo repository, and pull request #37 of the dojo-core repository.
The upgradeable.cairo contract has a comment that says "is" instead of "if".
Consider correcting the above-mentioned typographical error and thoroughly examining the codebase to rectify any additional ones to improve the quality and readability of the codebase.
Update: Resolved in pull request #2641 of the dojo repository, and pull request #36 of the dojo-core repository.
This diff audit only covered the major changes made to the originally audited codebase, with the primary change being the ability to pass the namespace as a parameter when registering or updating events, models, or systems, instead of requesting it from the corresponding contract. We made several recommendations to improve the overall quality of the codebase. The Cartridge Dojo engine team was helpful and responsive to our questions throughout the diff audit process.