hats-finance / Circles-0x6ca9ca24d78af44582951825bef9eadcb210e5cf

Circles Protocol contracts
https://aboutcircles.com
GNU Affero General Public License v3.0
0 stars 0 forks source link

Stopped profile can still be used for registration #19

Open hats-bug-reporter[bot] opened 1 week ago

hats-bug-reporter[bot] commented 1 week ago

Github username: -- Twitter username: -- Submission hash (on-chain): 0x367b5d1e3734d24f7d11de72905edcae3013a2905d6309415364b6b302e5c444 Severity: low

Description:

Description

Even though a user stops their avatar via stop() if they trusted someone previously, the trusted user could still register with burning the inviter's INVITATION_COST and getting WELCOME_BONUS tokens.

Proof of Concept

  1. Navigate to test/hub/V1MintStatusUpdate.t.sol
  2. add the below migrationSetup() and testStopAndRegister() functions
  3. run forge test --mt testStopAndRegister -vvvv

    function migrationSetup() public {
        // Alice and Bob register in V1
        ITokenV1 tokenAlice = signupInV1(addresses[0]);
        ITokenV1 tokenBob = signupInV1(addresses[1]);
    
        // move time
        skipTime(5 days + 1 hours + 31 minutes);
    
        // Alice and Bob mint in V1
        mintV1Tokens(tokenAlice);
        mintV1Tokens(tokenBob);
    
        // Alice stops her V1 token and registers in V2
        vm.startPrank(addresses[0]);
        tokenAlice.stop();
        assertTrue(tokenAlice.stopped(), "Token not stopped");
        mockHub.registerHuman(address(0), bytes32(0));
        vm.stopPrank();
        assertTrue(mockHub.isHuman(addresses[0]), "Alice not registered");
    
        // Alice invites Bob, while he is still active in V1
        // to do that she must trust Bob, and then he can register with Alice as inviter
        vm.prank(addresses[0]);
        // Alice trusts Bob
        mockHub.trust(addresses[1], type(uint96).max);
        // Bob registers with Alice as inviter
        vm.prank(addresses[1]);
        // Bob calls register Human with Alice as inviter
        mockHub.registerHuman(addresses[0], bytes32(0));
        assertTrue(mockHub.isHuman(addresses[1]), "Bob not registered");
    
        // move time
        skipTime(5 days - 31 minutes);
    
        // Alice can mint in V2
        vm.prank(addresses[0]);
        mockHub.personalMint();
    
        // Bob cannot mint in V2, but can in V1
        vm.expectRevert();
        vm.prank(addresses[1]);
        mockHub.personalMint();
        mintV1Tokens(tokenBob);
    
        // Bob stops his V1 token
        vm.prank(addresses[1]);
        tokenBob.stop();
    
        // Bob can mint in V2, but calculateIssuance will still revert because V1 status not updated
        vm.expectRevert();
        mockHub.calculateIssuance(addresses[1]);
        // however, sending a transaction to update the V1 status does calculate the issuance, and update status
        mockHub.calculateIssuanceWithCheck(addresses[1]);
    
        // move time
        skipTime(5 days + 31 minutes);
    }
    
    function testStopAndRegister() public {
        // alice trusts bob in the setup
        migrationSetup();
    
        // alice stops her token
        vm.prank(addresses[0]);
        mockHub.stop();
    
        // bob can still mint in V2 despite alice's token shut down 
        vm.prank(addresses[1]);
        mockHub.personalMint();
    }

Recommendation

Consider to disallow registrations with stopped profiles's avatar currencies by checking if the inviters lastMintTime is set to INDEFINITE_FUTURE and reverting registerHuman if it is.

benjaminbollen commented 1 week ago

Thank you for your detailed report on the ability to use a stopped profile for registration. We appreciate the effort you put into writing a test to demonstrate this behavior. After review, we've determined that this is not an issue, but rather an intended feature of our system. Here's why:

  1. Stopping a v1 or v2 profile only halts token minting, not all functionality.
  2. For v2 self-registration, it's actually required that the v1 avatar be stopped first.
  3. The 'stop' function is not meant to completely "shut down" (as per your comment in the test) the token, but to stop the person of minting more of their Circles.

Your thorough analysis and test creation demonstrate a deep engagement with our system. While this behavior is intentional, your report helps ensure our design choices are clear and well-documented. Thank you for contributing to the robustness of our platform.