Potential extension of membership outside the grace period

[romanzac]

The set of assertions registerMembership, attemptExtensionRace, attemptErasureRace was used to simulate RLN smart contract behavior with Echidna. During simulation both attemptExtensionRace and attemptErasureRace had each an instance with invalid result. It seems the contract allows extension of a membership outside the grace period when many users attempt to do so.

Impact

Low occurrence, medium impact.

To reproduce from replay

1. Checkout 0238995
2. cd waku-rlnv2-contract
3. forge test --match-test test_attemptExtensionRace_WakuRLN -vvv or forge test --match-test test_attemptErasureRace_WakuRLN -vvv

To reproduce from Echidna run

1. Checkout 657c917
2. cd waku-rlnv2-contract
3. echidna test/EchidnaTestRaces.t.sol --contract EchidnaTestRaces --config echidna.config.yaml --format json > echidna_log.json
4. Go to https://getrecon.xyz/tools/echidna and paste echidna_log.json content
5. Copy the converted test cases into replay contract.
6. Fix issues unrelated to membership extension (invalid commitment, high GAS consumption) - use AI to help
7. forge test --match-test test_attemptExtensionRace_WakuRLN -vvv or forge test --match-test test_attemptErasureRace_WakuRLN -vvv

Expected behavior

RLN contract should ideally never allow membership extension outside of the grace period.

Screenshots/logs

echidna_log.json
test_attemptExtensionRace_WakuRLN.log
test_attemptErasureRace_Waku.log

[romanzac]

During further investigation, I found problems with assertions order. When the assertions are performed after the w.eraseMemberships smart contract is already in a different state. After fixing the test at b060b00 Echidna was not able to falsify any assertion.

Original:

        uint256[] memory ids = new uint256[](1);
        ids[0] = focusId;

        bool success = false;
        try w.eraseMemberships(ids, fullErase) {
            success = true;
        } catch { }

        // Assertion: Erasure should succeed only if not active (i.e., in grace or expired)
        assert(success == !isActive);

        // Additional assertions: State consistency
        assert(w.isExpired(focusId) == isExpired);
        assert(!w.isInGracePeriod(focusId) == (isExpired || isActive));

Fixed:

        // Additional assertions: State consistency (pre-operation)
        assert(w.isExpired(focusId) == isExpired);
        assert(w.isInGracePeriod(focusId) == !(isExpired || isActive));

        uint256[] memory ids = new uint256[](1);
        ids[0] = focusId;

        bool success = false;
        try w.eraseMemberships(ids, fullErase) {
            success = true;
        } catch { }

        // Assertion: Erasure should succeed only if not active (i.e., in grace or expired)
        // (and for grace, sender == holder, but always true here)
        assert(success == !isActive);