diff --git a/src/Membership.sol b/src/Membership.sol index 27fc29f..7157723 100644 --- a/src/Membership.sol +++ b/src/Membership.sol @@ -19,8 +19,11 @@ error CannotExtendActiveMembership(uint256 idCommitment); // The sender is not the holder of this membership error NonHolderCannotExtend(uint256 idCommitment); -// This membership cannot be erased -error CannotEraseMembership(uint256 idCommitment); +// The membership is still active +error CannotEraseActiveMembership(uint256 idCommitment); + +// The sender is not the holder of this membership +error NonHolderCannotErase(uint256 idCommitment); // This membership does not exist error MembershipDoesNotExist(uint256 idCommitment); @@ -264,8 +267,10 @@ abstract contract MembershipUpgradeable is Initializable { _isInPeriod(membership.gracePeriodStartTimestamp, membership.gracePeriodDuration); bool isHolder = (membership.holder == _sender); - if (!membershipExpired && !(membershipIsInGracePeriod && isHolder)) { - revert CannotEraseMembership(_idCommitment); + if (!membershipExpired && !membershipIsInGracePeriod) { + revert CannotEraseActiveMembership(_idCommitment); + } else if (membershipIsInGracePeriod && !isHolder) { + revert NonHolderCannotErase(_idCommitment); } // Move deposit balance from the membership to be erased to holder deposit balance diff --git a/test/WakuRlnV2.t.sol b/test/WakuRlnV2.t.sol index cacd7a8..6f0d609 100644 --- a/test/WakuRlnV2.t.sol +++ b/test/WakuRlnV2.t.sol @@ -378,7 +378,7 @@ contract WakuRlnV2Test is Test { commitmentsToErase[1] = 2; commitmentsToErase[2] = 5; // This one is still active token.approve(address(w), priceB); - vm.expectRevert(abi.encodeWithSelector(CannotEraseMembership.selector, 5)); + vm.expectRevert(abi.encodeWithSelector(CannotEraseActiveMembership.selector, 5)); w.register(6, 60, commitmentsToErase); // Attempt to erase 3 memberships that can be erased @@ -463,14 +463,35 @@ contract WakuRlnV2Test is Test { (, uint256 price) = w.priceCalculator().calculate(20); uint32 index; uint256[] memory commitmentsToErase = new uint256[](idCommitmentsLength); + uint256 time = block.timestamp; for (uint256 i = 1; i <= idCommitmentsLength; i++) { token.approve(address(w), price); w.register(i, 20, noCommitments); (,,,,, index,,) = w.memberships(i); assertEq(index, w.nextFreeIndex() - 1); commitmentsToErase[i - 1] = i; + time += 100; + vm.warp(time); + } + + // None of the commitments can be deleted because they're still active + uint256[] memory singleCommitmentToErase = new uint256[](1); + for (uint256 i = 1; i <= idCommitmentsLength; i++) { + singleCommitmentToErase[0] = i; + vm.expectRevert(abi.encodeWithSelector(CannotEraseActiveMembership.selector, i)); + w.eraseMemberships(singleCommitmentToErase); } + // Fastfwd to commitment grace period, and try to erase it without being the owner + (,, uint256 gracePeriodStartTimestamp,,,,,) = w.memberships(1); + vm.warp(gracePeriodStartTimestamp); + assertTrue(w.isInGracePeriod(1)); + singleCommitmentToErase[0] = 1; + address randomAddress = vm.addr(block.timestamp); + vm.prank(randomAddress); + vm.expectRevert(abi.encodeWithSelector(NonHolderCannotErase.selector, 1)); + w.eraseMemberships(singleCommitmentToErase); + // time travel to the moment we can erase all expired memberships uint256 membershipExpirationTimestamp = w.membershipExpirationTimestamp(idCommitmentsLength); vm.warp(membershipExpirationTimestamp); @@ -562,7 +583,7 @@ contract WakuRlnV2Test is Test { vm.warp(gracePeriodStartTimestamp - 1); commitmentsToErase[0] = idCommitment; commitmentsToErase[1] = idCommitment + 4; - vm.expectRevert(abi.encodeWithSelector(CannotEraseMembership.selector, idCommitment + 4)); + vm.expectRevert(abi.encodeWithSelector(CannotEraseActiveMembership.selector, idCommitment + 4)); w.eraseMemberships(commitmentsToErase); }