Auth does not verify functions using the isAuthorized modifier as expected when called internally

[kingofclubstroyDev]

Auth uses the msg.sig value to determine the function selector of the function being called and verifies it with the Authority contract, if set. However, msg.sig returns the function selector of the initial function call in the context of execution, and not necessarily the function selector of the function that calls isAuthorized.
There are two cases where this distinction may come up:

1. If public function with the isAuthorized modifier is called internally, the msg.sig it will check is the external function that called it. This is also the case for the for the setAuthority and transferOwnership functions.
2. If an internal function uses the isAuthorized modifier, it will also only verify the external function. This is not a common pattern but is sometimes used.

The cases where this can be an issue are fairly rare, but developers could make assumptions and incorrectly authorize important functions which could lead to quite severe implications.

It is suggested that a notice is added to notify developers if this behaviour, because it is not directly apparent. Additionally you could add another modifier with a function selector input parameter to allow users control over which selector is verified.

Here is a sample implementation and corresponding forge test:

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;
import {console} from "forge-std/console.sol";
import {Auth, Authority} from "lib/solmate/src/auth/Auth.sol";

contract AuthImplementation is Auth {
    uint256 public number;

    bytes4 shouldNotBeCalledByAnyoneInternalSelector = bytes4(keccak256(bytes("shouldNotBeCalledByAnyoneInternal()")));

    constructor(address owner) Auth(owner, Authority(address(this))) {}


    function shouldNotBeCalledByAnyoneInternal() requiresAuth internal {}

    function shouldNotBeCalledByAnyonePublic() requiresAuth public {}

    function externalCall() requiresAuth external {

        // these should not be callable by anyone other than the owner, but because an external function initiated it, the msg.sig == externalCall.selector 
        // rather than the expected shouldNotBeCalledByAnyoneInternalSelector or shouldNotBeCalledByAnyonePublic.selector
        shouldNotBeCalledByAnyoneInternal();
        shouldNotBeCalledByAnyonePublic();

    }

    function externalTransferOwnership(address newOwner) requiresAuth external {

        transferOwnership(newOwner);
        require(msg.sender == owner);

    }

    // contract is it's own authority
    function canCall(
        address user,
        address target,
        bytes4 functionSig
    ) external view returns (bool) {

        // anyone can call 
        if(functionSig == this.externalTransferOwnership.selector) {
            return true;
        }

        // non owner cannot call
        if(functionSig == shouldNotBeCalledByAnyoneInternalSelector) {
            // note this will never be reached, since it always uses the external function selector
            return false;
        }

        if(functionSig == this.shouldNotBeCalledByAnyonePublic.selector) {
            return false;
        }

        // non owner cannot transfer ownership
        if(functionSig == this.transferOwnership.selector) {
            return false;
        }

        return true;

    }


}

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console} from "forge-std/Test.sol";
import {AuthImplementation, Authority} from "../src/AuthImplementation.sol";

contract AuthTest is Test {
    AuthImplementation public authImplementation;
    address bob = vm.addr(0xDAD);
    address alice = vm.addr(0xFAE);
    address attacker = vm.addr(0xDEAD);

    function setUp() public {
        authImplementation = new AuthImplementation(bob);
    }

    function test_nonOwnerCanTransferOwnership() public {

        // transfers ownership normally
        vm.prank(bob);
        authImplementation.transferOwnership(alice);

        assertEq(authImplementation.owner(), alice);

        // bob can no longer transfer ownership
        vm.expectRevert();
        vm.prank(bob);
        authImplementation.transferOwnership(bob);

        // the attacker cannot transfer ownership normally
        vm.expectRevert();
        vm.prank(attacker);
        authImplementation.transferOwnership(attacker);

        // they can however do it through the external function that anyone can call, which calls transferOwnership internally
        vm.prank(attacker);
        authImplementation.externalTransferOwnership(attacker);

        assertEq(authImplementation.owner(), attacker);


    }

    function test_canCallAuthorizedFunctionsInternallyWithoutAuthorization() public {

        // owner can call all isauthorized functions
        vm.prank(bob);
        authImplementation.shouldNotBeCalledByAnyonePublic();

        // attacker cannot directly call
        vm.expectRevert();
        vm.prank(attacker);
        authImplementation.shouldNotBeCalledByAnyonePublic();

        // they can however do it through the external function that anyone can call
        vm.prank(attacker);
        authImplementation.externalCall();


    }
   
}