unlimiting the delay period with minimum threshold allow the delay period to be set too low and allow a malicious ScheduleUpgrade message to be executed on the root contract and gain access on the other contracts.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/Root.sol#L43-L50
https://github.com/code-423n4/2023-09-centrifuge/blob/512e7a71ebd9ae76384f837204216f26380c9f91/src/Root.sol#L16-L17

Vulnerability details

Impact

allowing to modify the delay period without minimum threshold can result in set the delay period to a too short period or even zero ,which is allowed according to the code , and this could lead to allowing a malicious ScheduleUpgrade message to pass and a malicious account can gain control over all the contracts of the protocols.

Proof of Concept

in the Root.sol contract , the protocol implements a maximum threshold in the function file() which can modify the delay variable , so the possibility of the set the delay period to a very long period exists and also setting the delay period to a very short period is still possible ,as shown in the file() which will not prevent set the delay period to zero or any short period :

    function file(bytes32 what, uint256 data) external auth {
        if (what == "delay") {
            require(data <= MAX_DELAY, "Root/delay-too-long");
            delay = data;
        } else {
            revert("Root/file-unrecognized-param");
        }
        emit File(what, data);

so if the delay was set to a very short period and Someone gains control over a router and triggers a malicious ScheduleUpgrade message , the malicious attacker can trigger the executeScheduledRely() function after the delay period , which is too short , to give his address or any contract an auth role on any of the protocol contracts , the attacker can be auth on the escrow contract and steals all the funds from the protocol .

this attack does not assume that the auth admin on the Root contract is malicious , but it is all about the possibility of setting the delay period to a very short period or even zero , which can be happened mistakenly or intentionally .

Tools Used

manual review

Recommended Mitigation Steps

this vulnerability can be mitigated by implementing a minimum threshold MIN_DELAY to the delay period , so this will prevent setting the delay period to zero or even a very short period .

+    uint256 private MIN_DELAY = 2 days ;

    function file(bytes32 what, uint256 data) external auth {
        if (what == "delay") {
            require(data <= MAX_DELAY, "Root/delay-too-long");
+           require(data >= MIN_DELAY, "Root/delay-too-short");

            delay = data;
        } else {
            revert("Root/file-unrecognized-param");
        }
        emit File(what, data);

Assessed type

Invalid Validation

[c4-pre-sort]

raymondfam marked the issue as low quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-judge]

gzeon-c4 changed the severity to QA (Quality Assurance)