From f6ee779498c3ad545cedbcbc13269f2e62f12845 Mon Sep 17 00:00:00 2001
From: Crystal Lemire <crystal@hntlabs.com>
Date: Thu, 3 Oct 2024 15:22:44 -0700
Subject: [PATCH] [TOWNS-12637] Update RuleEntitlementV2 to revert when zero
 length rule 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.
---
 .../entitlements/rule/RuleEntitlementV2.sol   | 12 +++++++++--
 .../test/crosschain/RuleEntitlementV2.t.sol   | 21 +++++++++++++++++++
 2 files changed, 31 insertions(+), 2 deletions(-)

diff --git a/contracts/src/spaces/entitlements/rule/RuleEntitlementV2.sol b/contracts/src/spaces/entitlements/rule/RuleEntitlementV2.sol
index 483d29097..438ff5872 100644
--- a/contracts/src/spaces/entitlements/rule/RuleEntitlementV2.sol
+++ b/contracts/src/spaces/entitlements/rule/RuleEntitlementV2.sol
@@ -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;
@@ -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;
diff --git a/contracts/test/crosschain/RuleEntitlementV2.t.sol b/contracts/test/crosschain/RuleEntitlementV2.t.sol
index 83e8dbace..bd1bb2f43 100644
--- a/contracts/test/crosschain/RuleEntitlementV2.t.sol
+++ b/contracts/test/crosschain/RuleEntitlementV2.t.sol
@@ -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();