code-423n4 / 2023-09-centrifuge-findings

16 stars 14 forks source link

The executeScheduledRely function in the code allows any external caller to execute it without proper access control checks. This means that anyone can make themselves a ward on a contract without authorization, which poses a security risk. #757

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

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

Vulnerability details

Issue:

The executeScheduledRely function in the code allows any external caller to execute it without proper access control checks. This means that anyone can make themselves a ward on a contract without authorization, which poses a security risk.

Proof of Concept:

Without proper access control, an attacker can call the executeScheduledRely function as follows:

rootContract.executeScheduledRely(targetContract);

This allows the attacker to become a ward on the targetContract without authorization.

Code Block Examples:

Current Implementation:

function executeScheduledRely(address target) public {
    require(schedule[target] != 0, "Root/target-not-scheduled");
    require(schedule[target] < block.timestamp, "Root/target-not-ready");

    wards[target] = 1;
    emit Rely(target);

    schedule[target] = 0;
}

Recommended Mitigation:

// Add a modifier to restrict access to authorized users (e.g., the contract owner).
modifier onlyAuthorized() {
    require(wards[msg.sender] == 1, "Root/not-authorized");
    _;
}

function executeScheduledRely(address target) public onlyAuthorized {
    require(schedule[target] != 0, "Root/target-not-scheduled");
    require(schedule[target] < block.timestamp, "Root/target-not-ready");

    wards[target] = 1;
    emit Rely(target);

    schedule[target] = 0;
}

Explanation:

  1. We've introduced an onlyAuthorized modifier to restrict access to authorized users.
  2. The onlyAuthorized modifier checks if the sender (caller) is authorized based on the wards mapping.
  3. In the executeScheduledRely function, we've applied the onlyAuthorized modifier to ensure that only authorized users can execute it.

Assessed type

Access Control

c4-pre-sort commented 1 year ago

raymondfam marked the issue as low quality report

c4-pre-sort commented 1 year ago

raymondfam marked the issue as duplicate of #635

c4-judge commented 1 year ago

gzeon-c4 marked the issue as unsatisfactory: Invalid