External visibility modifier on function that should be callable from address(this). Doesnt seem right.

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-moonwell/blob/8694244ebf607a4ed33c0b74f422019fe8eb8d3e/src/core/Governance/TemporalGovernor.sol#L187-L198

Vulnerability details

Impact

External visibility modifier on function that should be callable from address(this). Doesnt seem right.

Proof of Concept

/// @notice grant the guardians the pause ability
function grantGuardiansPause() external {
    require(
        msg.sender == address(this),    /// @audit is this right?
        "TemporalGovernor: Only this contract can update grant guardian pause"
    );

    guardianPauseAllowed = true;
    lastPauseTime = 0;

    emit GuardianPauseGranted(block.timestamp);
}

Tools Used

VSC, manual.

Recommended Mitigation Steps

The msg.sender part should not be address(this), as is currently the case: msg.sender == address(this)

Unless I'm mistaken, the above should not be possible?

Assessed type

Access Control

[c4-pre-sort]

0xSorryNotSorry marked the issue as primary issue

[ElliotFriedman]

this should not be a primary issue, see unit tests for expected behavior. It's how a timelock modifies itself through a self call.

[c4-sponsor]

ElliotFriedman marked the issue as sponsor disputed

[alcueca]

msg.sender is updated in each external call. The Temporal governor calling this.grantGuardianPause() will mean that msg.sender == address(this)

[c4-judge]

alcueca marked the issue as unsatisfactory: Invalid