Skip to content

Commit

Permalink
[TOWNS-12637] Update RuleEntitlementV2 to revert when zero length rul…
Browse files Browse the repository at this point in the history
…e data is passed in (#1218)

Previously setEntitlement RuleEntitlementV2 ignored empty rule datas,
but this created a bit of confusion with createSpace when the architect
assumed a rule data was set when creating the minter role.

Here we update the contract to just always revert when setEntitlement
doesn't actually set an entitlement as an extra guard to avoid creating
spaces with an invalid view of entitlements on the minter role.
  • Loading branch information
clemire authored Oct 3, 2024
1 parent 653851b commit f6ee779
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 2 deletions.
12 changes: 10 additions & 2 deletions contracts/src/spaces/entitlements/rule/RuleEntitlementV2.sol
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,11 @@ contract RuleEntitlementV2 is
) external onlySpace {
_removeRuleDataV1(roleId);

if (entitlementData.length == 0) return;
// We should never allow the setting of empty rule datas because it can cause the xchain
// architecture to be invoked when by default a user is not entitled.
if (entitlementData.length == 0) {
revert Entitlement__InvalidValue();
}

// equivalent: abi.decode(entitlementData, (RuleDataV2))
RuleDataV2 calldata data;
Expand All @@ -127,7 +131,11 @@ contract RuleEntitlementV2 is
data := add(entitlementData.offset, calldataload(entitlementData.offset))
}

if (data.operations.length == 0) return;
// We should never allow the setting of empty rule datas because it can cause the xchain
// architecture to be invoked when by default a user is not entitled.
if (data.operations.length == 0) {
revert Entitlement__InvalidValue();
}

// Cache lengths of operations arrays to reduce state access cost
uint256 operationsLength = data.operations.length;
Expand Down
21 changes: 21 additions & 0 deletions contracts/test/crosschain/RuleEntitlementV2.t.sol
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,27 @@ contract RuleEntitlementV2Test is RuleEntitlementTest {
assertEq(vm.getMappingLength(entitlement, bytes32(ENTITLEMENTS_SLOT)), 0);
}

function test_setRuleEntitlement_revertOnEmptyRuleData() public {
test_upgradeToRuleV2();

vm.expectRevert(Entitlement__InvalidValue.selector);

vm.prank(space);

ruleEntitlementV2.setEntitlement(roleId, "");
}

function test_setRuleEntitlement_revertOnZeroLengthRuleData() public {
test_upgradeToRuleV2();

vm.expectRevert(Entitlement__InvalidValue.selector);

vm.prank(space);

RuleDataV2 memory ruleDataV2;
ruleEntitlementV2.setEntitlement(roleId, abi.encode(ruleDataV2));
}

function test_removeRuleEntitlement() external override {
test_setRuleEntitlement();

Expand Down

0 comments on commit f6ee779

Please sign in to comment.