CM can exploit a pause in `GuardCM` to gain permanent unrestricted access

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/governance/contracts/multisigs/GuardCM.sol#L427-L429 https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/governance/contracts/multisigs/GuardCM.sol#L400

Vulnerability details

Impact

The GuardCM contract is designed to restrict the Community Multisig (CM) actions within the protocol to only specific contracts and methods. Under specific circumstances, the protocol allows the guard to be paused, which temporarily pauses the guard and provides the CM with full access to the timelock.

However, a potential vulnerability in the current implementation allows the CM to exploit a pause in GuardCM to gain permanent unrestricted access. This is because self-calls are only disallowed when the contract is not paused. If it is paused, checkTransaction simply always passes. This means that the CM can take advantage of an event that causes governance to pause the guard, call setGuard() on itself and remove the guard altogether. This would effectively give the CM permanent unrestricted access, bypassing the guard's restrictions.

The CM holds all privileged roles within the timelock, which is the protocol owner. This means that the CM can potentially gain permanent unrestricted control over the entire protocol. As such, this vulnerability represents a significant risk of privilege escalation and is classified as high severity.

Proof of Concept

The following sequence of events could lead to the CM gaining unrestricted access:

1. An event occurs that causes the governance to pause the GuardCM contract.
2. While the GuardCM contract is paused, the CM executes setGuard(0) on itself.
3. The setGuard() function removes the guard from the CM.
4. The CM now has permanent unrestricted access, bypassing the guard's restrictions.

Tools Used

Manual review

Recommended Mitigation Steps

To mitigate this issue, it is recommended to implement checks for self-calls even when the contract is paused. This would prevent the CM from being able to call the setGuard() function on itself and remove the guard:

@@ -397,7 +397,10 @@ contract GuardCM {
         bytes memory,
         address
     ) external {
-        // Just return if paused
+        if (to == multisig) {
+            // No self multisig call is allowed
+            revert NoSelfCall();
+        }
         if (paused == 1) {
             // Call to the timelock
             if (to == owner) {
@@ -424,9 +427,6 @@ contract GuardCM {

                     _verifySchedule(data, functionSig);
                 }
-            } else if (to == multisig) {
-                // No self multisig call is allowed
-                revert NoSelfCall();
             }
         }

Assessed type

Access Control

[c4-pre-sort]

alex-ppg marked the issue as primary issue

[c4-pre-sort]

alex-ppg marked the issue as insufficient quality report

[c4-sponsor]

kupermind (sponsor) disputed

[c4-judge]

dmvt marked the issue as unsatisfactory: Invalid

[MarioPoneder]

Dear Judge, Due to #437 being valid, it seems that the present issue was disputed/invalidated by accident which is pretty understandable as it has also happened to me when judging.

Nevertheless, a few aspects to provide an overview:

• The actions of the CM (community multisig) are restricted by the guard (GuardCM contract). This guard has an (un-)pausing mechanism that temporarily allows the CM unrestricted control over the protocol.
• The present issue allows the CM to simply remove that guard (similar to #437) with the pre-condition that the guard has to be paused at least once. See also #162 for a runnable PoC.
• However, once the guard is paused and the CM has removed it, the unpausing mechansism becomes ineffective and leaves the CM with unrestricted control over the protocol.
• This defies the purpose of having a guard in the first place. As of now, the governance is misled to believe that the CM's powers are just temporary and can be revoked via unpausing, while in reality it solely depends on the CM's goodwill to keep the guard.
• For reference, please see also reNFT's guard check which again shows the validity of the present concern.

Kind regards,
0xTheC0der

[dmvt]

https://docs.code4rena.com/awarding/judging-criteria/supreme-court-decisions-fall-2023#verdict-severity-standardization-centralization-risks

Privilege escalation issues are judged by likelihood and impact and their severity is uncapped.

For both issues, we have to assume that the CM is maliciously controlled, which is unlikely IMO. However, with #437 if at any point it is, there is no way to prevent it from taking full control and no time to notice that this has happened. With this issue, the CM has to be malicious, and unnoticed until an event severe enough to trigger a pause. Then it needs to act quickly and collect signatures prior to an unpause. I view this as a highly unlikely string of events. IMO, this issue should have been included as a QA or admin risk in Analysis.