First Flight #16: Mafia Takedown

Summary

The ReadMe states that the policy contract "Laundrette.sol" is "the admin of Kernel.sol to grant and revoke roles. A function permit the godfather to retrieve the admin role when needed." The function provided in the policy for the godfather to retrieve the admin role is retrieveAdmin() :

function retrieveAdmin() external {
    kernel.executeAction(Actions.ChangeAdmin, kernel.executor());
}

This function when called attempts to change the admin of the kernel to kernel.executor(), which is set to the godFather's address at deployment. However, this function will never be able to set the admin of the kernel to kernel.executor(). This is because kernel.executeAction() is protected by the onlyExecutor modifier :

// Role reserved for governor or any executing address
modifier onlyExecutor() {
    if (msg.sender != executor) revert Kernel_OnlyExecutor(msg.sender);
    _;
}

This modifier checks that the address that is calling this function is equal to the executor. However, since laundrette.retrieveAdmin() function calls the kernel.executeAction() function, the msg.sender that is checked in the onlyExecutor() contract will always be the address of the policy laundrette. Therefore, the function laundrette.retrieveAdmin() will never work, even when called by the "godFather".

Vulnerability Details

Here is a quick proof of concept :

error Kernel_OnlyExecutor(address caller_);

function test_retrieveAdmin() public {
    console.log("executor : ", kernel.executor());
    console.log("admin : ", kernel.admin());
    console.log("godFather : ", godFather);
    console.log(" ");

    //1) Try to retrieve the admin through the policy 
    vm.expectRevert(abi.encodeWithSelector(Kernel_OnlyExecutor.selector, address(laundrette)));
    vm.prank(godFather);
    laundrette.retrieveAdmin();
    assertNotEq(godFather, kernel.admin());

    console.log("executor : ", kernel.executor());
    console.log("admin : ", kernel.admin());
    console.log("godFather : ", godFather);
    console.log(" ");

    //2) Try directly calling the kernel to set the admin
    vm.prank(godFather);
    kernel.executeAction(Actions.ChangeAdmin, godFather);
    assertEq(godFather, kernel.admin());

    console.log("executor : ", kernel.executor());
    console.log("admin : ", kernel.admin());
    console.log("godFather : ", godFather);
    console.log(" ");
}

Impact

The laundrette.retrieveAdmin() does not work as intended. It will fail at changing the admin in the kernel unless the policy becomes the executor. However, the executor can directly call the kernel if it wishes to become the admin as well.

Tools Used

Foundry

Recommendations

Just remove the laundrette.retrieveAdmin() function. It does not work as intended. If the godFather would like to retrieve the Admin, it can just call the kernel directly.