Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(store,world): rename ambiguous elements [N-03] #2091

Merged
merged 5 commits into from
Jan 12, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions packages/store/src/FieldLayout.sol
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,20 @@ library FieldLayoutLib {
* @notice Encodes the given field layout into a single bytes32.
* @dev Ensures various constraints on the length and size of the fields.
* Reverts if total fields, static field length, or static byte length exceed allowed limits.
* @param _staticFields An array of static field lengths.
* @param _staticFieldLengths An array of static field lengths.
* @param numDynamicFields The number of dynamic fields.
* @return A FieldLayout structure containing the encoded field layout.
*/
function encode(uint256[] memory _staticFields, uint256 numDynamicFields) internal pure returns (FieldLayout) {
function encode(uint256[] memory _staticFieldLengths, uint256 numDynamicFields) internal pure returns (FieldLayout) {
uint256 fieldLayout;
uint256 totalLength;
uint256 totalFields = _staticFields.length + numDynamicFields;
uint256 totalFields = _staticFieldLengths.length + numDynamicFields;
if (totalFields > MAX_TOTAL_FIELDS) revert FieldLayoutLib_InvalidLength(totalFields);
if (numDynamicFields > MAX_DYNAMIC_FIELDS) revert FieldLayoutLib_InvalidLength(numDynamicFields);

// Compute the total static length and store the field lengths in the encoded fieldLayout
for (uint256 i = 0; i < _staticFields.length; ) {
uint256 staticByteLength = _staticFields[i];
for (uint256 i = 0; i < _staticFieldLengths.length; ) {
uint256 staticByteLength = _staticFieldLengths[i];
if (staticByteLength == 0) {
revert FieldLayoutLib_StaticLengthIsZero();
} else if (staticByteLength > WORD_SIZE) {
Expand All @@ -58,7 +58,7 @@ library FieldLayoutLib {
totalLength += staticByteLength;
// Sequentially store lengths after the first 4 bytes (which are reserved for total length and field numbers)
// (safe because of the initial _staticFields.length check)
fieldLayout |= uint256(_staticFields[i]) << ((WORD_LAST_INDEX - 4 - i) * BYTE_TO_BITS);
fieldLayout |= uint256(_staticFieldLengths[i]) << ((WORD_LAST_INDEX - 4 - i) * BYTE_TO_BITS);
i++;
}
}
Expand All @@ -68,7 +68,7 @@ library FieldLayoutLib {
// number of dynamic fields in the 4th byte
// (optimizer can handle this, no need for unchecked or single-line assignment)
fieldLayout |= totalLength << LayoutOffsets.TOTAL_LENGTH;
fieldLayout |= _staticFields.length << LayoutOffsets.NUM_STATIC_FIELDS;
fieldLayout |= _staticFieldLengths.length << LayoutOffsets.NUM_STATIC_FIELDS;
fieldLayout |= numDynamicFields << LayoutOffsets.NUM_DYNAMIC_FIELDS;

return FieldLayout.wrap(bytes32(fieldLayout));
Expand Down
8 changes: 4 additions & 4 deletions packages/store/src/Hook.sol
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,15 @@ library HookLib {
* @notice Filter a hook from the hook list by its address.
* @dev This function writes the updated hook list to the table in place.
* @param hookTableId The resource ID of the hook table.
* @param tableWithHooks The resource ID of the table with hooks to filter.
* @param elementWithHooks The resource ID of the table with hooks to filter.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we name this resourceWithHooks?

"element" is a term we've never used before and I think "resource" is more consistent: https://www.notion.so/latticexyz/MUD-naming-conventions-terminology-66e11356a1934465b2aa1a951c61a299

* @param hookAddressToRemove The address of the hook to remove.
*/
function filterListByAddress(
ResourceId hookTableId,
ResourceId tableWithHooks,
ResourceId elementWithHooks,
address hookAddressToRemove
) internal {
bytes21[] memory currentHooks = Hooks._get(hookTableId, tableWithHooks);
bytes21[] memory currentHooks = Hooks._get(hookTableId, elementWithHooks);

// Initialize the new hooks array with the same length because we don't know if the hook is registered yet
bytes21[] memory newHooks = new bytes21[](currentHooks.length);
Expand All @@ -61,7 +61,7 @@ library HookLib {
}

// Set the new hooks table
Hooks._set(hookTableId, tableWithHooks, newHooks);
Hooks._set(hookTableId, elementWithHooks, newHooks);
}
}

Expand Down
14 changes: 7 additions & 7 deletions packages/world/src/Create2.sol
Original file line number Diff line number Diff line change
Expand Up @@ -15,20 +15,20 @@ library Create2 {
*
* Requirements:
*
* - `bytecode` must not be empty.
* - `salt` must have not been used for `bytecode` already.
* - `creationCode` must not be empty.
* - `salt` must have not been used for `creationCode` already.
*
* @param byteCode The bytecode of the contract to be deployed.
* @param creationCode The bytecode of the contract to be deployed.
* @param salt A 256-bit value that, combined with the bytecode, determines the address.
* @return addr The address of the newly deployed contract.
* @dev If the CREATE2 fails, reverts
*/
function deploy(bytes memory byteCode, uint256 salt) internal returns (address addr) {
function deploy(bytes memory creationCode, uint256 salt) internal returns (address addr) {
assembly {
// byteCode is one word (0x20 bytes) with the length, followed by the actual
// code for the constructor. So the code starts at byteCode+0x20, and is mload(byteCode)
// creationCode is one word (0x20 bytes) with the length, followed by the actual
// code for the constructor. So the code starts at creationCode+0x20, and is mload(creationCode)
// bytes long.
addr := create2(0, add(byteCode, 0x20), mload(byteCode), salt)
addr := create2(0, add(creationCode, 0x20), mload(creationCode), salt)
if iszero(extcodesize(addr)) {
revert(0, 0)
}
Expand Down
32 changes: 16 additions & 16 deletions packages/world/src/World.sol
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ contract World is StoreData, IWorldKernel {
/**
* @dev Prevents the World contract from calling itself.
*/
modifier requireNoCallback() {
Copy link
Member

@holic holic Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not blocking but I feel like we could expand the docs here to explain why this modifier is here and important to be used, esp since this relates to a now-fixed vulnerability

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added issue here: #2106

modifier prohibitDirectCallback() {
if (msg.sender == address(this)) {
revert World_CallbackNotAllowed(msg.sig);
}
Expand All @@ -64,7 +64,7 @@ contract World is StoreData, IWorldKernel {
* @param coreModule The core module to initialize the World with.
* @dev Only the initial creator can initialize. This can be done only once.
*/
function initialize(IModule coreModule) public requireNoCallback {
function initialize(IModule coreModule) public prohibitDirectCallback {
// Only the initial creator of the World can initialize it
if (msg.sender != creator) {
revert World_AccessDenied(ROOT_NAMESPACE_ID.toString(), msg.sender);
Expand All @@ -85,7 +85,7 @@ contract World is StoreData, IWorldKernel {
* @param args Arguments for module installation.
* @dev The caller must own the root namespace.
*/
function installRootModule(IModule module, bytes memory args) public requireNoCallback {
function installRootModule(IModule module, bytes memory args) public prohibitDirectCallback {
AccessControl.requireOwner(ROOT_NAMESPACE_ID, msg.sender);
_installRootModule(module, args);
}
Expand Down Expand Up @@ -131,7 +131,7 @@ contract World is StoreData, IWorldKernel {
bytes calldata staticData,
PackedCounter encodedLengths,
bytes calldata dynamicData
) public virtual requireNoCallback {
) public virtual prohibitDirectCallback {
// Require access to the namespace or name
AccessControl.requireAccess(tableId, msg.sender);

Expand All @@ -151,7 +151,7 @@ contract World is StoreData, IWorldKernel {
bytes32[] calldata keyTuple,
uint48 start,
bytes calldata data
) public virtual requireNoCallback {
) public virtual prohibitDirectCallback {
// Require access to the namespace or name
AccessControl.requireAccess(tableId, msg.sender);

Expand All @@ -175,7 +175,7 @@ contract World is StoreData, IWorldKernel {
uint40 startWithinField,
uint40 deleteCount,
bytes calldata data
) public virtual requireNoCallback {
) public virtual prohibitDirectCallback {
// Require access to the namespace or name
AccessControl.requireAccess(tableId, msg.sender);

Expand All @@ -195,7 +195,7 @@ contract World is StoreData, IWorldKernel {
bytes32[] calldata keyTuple,
uint8 fieldIndex,
bytes calldata data
) public virtual requireNoCallback {
) public virtual prohibitDirectCallback {
// Require access to namespace or name
AccessControl.requireAccess(tableId, msg.sender);

Expand All @@ -217,7 +217,7 @@ contract World is StoreData, IWorldKernel {
uint8 fieldIndex,
bytes calldata data,
FieldLayout fieldLayout
) public virtual requireNoCallback {
) public virtual prohibitDirectCallback {
// Require access to namespace or name
AccessControl.requireAccess(tableId, msg.sender);

Expand All @@ -240,7 +240,7 @@ contract World is StoreData, IWorldKernel {
uint8 fieldIndex,
bytes calldata data,
FieldLayout fieldLayout
) public virtual requireNoCallback {
) public virtual prohibitDirectCallback {
// Require access to namespace or name
AccessControl.requireAccess(tableId, msg.sender);

Expand All @@ -260,7 +260,7 @@ contract World is StoreData, IWorldKernel {
bytes32[] calldata keyTuple,
uint8 dynamicFieldIndex,
bytes calldata data
) public virtual requireNoCallback {
) public virtual prohibitDirectCallback {
// Require access to namespace or name
AccessControl.requireAccess(tableId, msg.sender);

Expand All @@ -280,7 +280,7 @@ contract World is StoreData, IWorldKernel {
bytes32[] calldata keyTuple,
uint8 dynamicFieldIndex,
bytes calldata dataToPush
) public virtual requireNoCallback {
) public virtual prohibitDirectCallback {
// Require access to namespace or name
AccessControl.requireAccess(tableId, msg.sender);

Expand All @@ -300,7 +300,7 @@ contract World is StoreData, IWorldKernel {
bytes32[] calldata keyTuple,
uint8 dynamicFieldIndex,
uint256 byteLengthToPop
) public virtual requireNoCallback {
) public virtual prohibitDirectCallback {
// Require access to namespace or name
AccessControl.requireAccess(tableId, msg.sender);

Expand All @@ -314,7 +314,7 @@ contract World is StoreData, IWorldKernel {
* @param keyTuple Array of keys identifying the record to delete.
* @dev Requires the caller to have access to the table's namespace or name.
*/
function deleteRecord(ResourceId tableId, bytes32[] calldata keyTuple) public virtual requireNoCallback {
function deleteRecord(ResourceId tableId, bytes32[] calldata keyTuple) public virtual prohibitDirectCallback {
// Require access to namespace or name
AccessControl.requireAccess(tableId, msg.sender);

Expand All @@ -338,7 +338,7 @@ contract World is StoreData, IWorldKernel {
function call(
ResourceId systemId,
bytes memory callData
) external payable virtual requireNoCallback returns (bytes memory) {
) external payable virtual prohibitDirectCallback returns (bytes memory) {
return SystemCall.callWithHooksOrRevert(msg.sender, systemId, callData, msg.value);
}

Expand All @@ -354,7 +354,7 @@ contract World is StoreData, IWorldKernel {
address delegator,
ResourceId systemId,
bytes memory callData
) external payable virtual requireNoCallback returns (bytes memory) {
) external payable virtual prohibitDirectCallback returns (bytes memory) {
// If the delegator is the caller, call the system directly
if (delegator == msg.sender) {
return SystemCall.callWithHooksOrRevert(msg.sender, systemId, callData, msg.value);
Expand Down Expand Up @@ -402,7 +402,7 @@ contract World is StoreData, IWorldKernel {
/**
* @dev Fallback function to call registered function selectors.
*/
fallback() external payable requireNoCallback {
fallback() external payable prohibitDirectCallback {
(ResourceId systemId, bytes4 systemFunctionSelector) = FunctionSelectors._get(msg.sig);

if (ResourceId.unwrap(systemId) == 0) revert World_FunctionSelectorNotFound(msg.sig);
Expand Down
Loading