Incorrect access control on setRange()

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/rwaOracles/RWADynamicOracle.sol#L154

Vulnerability details

Impact

In RWADynamicOracle.sol, setRange() function is used to set a given price range for USDY.

File: contracts/rwaOracles/RWADynamicOracle.sol

  function setRange(
    uint256 endTimestamp,
    uint256 dailyInterestRate
  ) external onlyRole(SETTER_ROLE) {
    Range memory lastRange = ranges[ranges.length - 1];

   
   // some code

  }

Here, it can be seen that this function can be accessed by address having SETTER_ROLE,however this is not correct role for this function per the contest readme.md. It states,

It is also important to note that when setting price's outside of the first range, the admin will only specify the Range.end and the Range.dailyInterestRate as the other parameters are calculated within the contract.

It basically mentions only admin can set the endTimestamp and dailyInterestRate. Also the setRange() function Natspec states,

@notice Function that allows for an admin to set a given price range for USDY

Here, It clearly mentions that setRange() should only be accessed by admin address. It is to be noted that DEFAULT_ADMIN_ROLE is granted role to admin address which can be checked here

Therefore, considering both the concrete references, setRange() should only be accessed by DEFAULT_ADMIN_ROLE.

Proof of Concept

https://github.com/code-423n4/2023-09-ondo/blob/47d34d6d4a5303af5f46e907ac2292e6a7745f6c/contracts/rwaOracles/RWADynamicOracle.sol#L154

Tools Used

Manual review

Recommended Mitigation Steps

DEFAULT_ADMIN_ROLE should only access setRange()

  function setRange(
    uint256 endTimestamp,
    uint256 dailyInterestRate
-  ) external onlyRole(SETTER_ROLE) {
+  ) external onlyRole(DEFAULT_ADMIN_ROLE) {
    Range memory lastRange = ranges[ranges.length - 1];


   // some code


  }

Assessed type

Access Control

[c4-pre-sort]

raymondfam marked the issue as duplicate of #25

[c4-pre-sort]

raymondfam marked the issue as low quality report

[raymondfam]

Known issue from readme.

[c4-judge]

kirk-baird marked the issue as unsatisfactory:
Invalid

[0xRizwan]

@kirk-baird

With due respect to judging decision,

This is incorrectly made duplicate of #25 and i dont agree with the presort reasoning Known issue from readme.

setRange() is one of the most critical function in RWADynamicOracle.sol which is used to set a price range for USDY. This issue is about the incorrect access control given to setRange() function with two concrete references from contest readme.md and function Natspec.

Reference 1: Function Natspec

>>>   * @notice Function that allows for an admin to set a given price range for USDY
   *
   * @param endTimestamp        The timestamp for the range to end
   * @param dailyInterestRate   The daily interest rate during said range
   */
  function setRange(

It means that setRange() can only be called by admin to set the price, however the current implementation has given access to setter role which seems to incorrect.

Reference 2: contest readme.md(RWADynamcRateOracle)

This contract will accept a Range as input from a trusted admin.

Another reference on setting range

It is also important to note that when setting price's outside of the first range, the admin will only specify the Range.end

It must also be noted that in RWADynamicOracle.sol the overrideRange() function can only be accessed by admin which is used to override a previously set range. This can be checked here

setting the range and overriding the range should only be handled by admin address which is the most trusted in ondo protocol, However, I am surprised to see only overrideRange() can be called by admin and not setRange().

I believe this should be Medium per the code4rena defination

2 — Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

Therefore, i believe considering the report and above escalation. The issue should be relooked. i highly trust the judge expertise and would accept the final decision on this issue.

Thank you!

[kirk-baird]

@mohammedrizwann123 I appreciate the time you've spent clarifying this issue. I agree with you that this should not be a duplicate of #25 and will adjust accordingly.

However, I do no see this as a Medium severity issue. I consider here the SETTER_ROLE to be an admin and therefore the code more or less matches the documentation. There could be some more clarification here in the documentations about which admin role may call the setRange() which task and so I'll leave this issue open as a QA.

[c4-judge]

kirk-baird marked the issue as not a duplicate

[c4-judge]

kirk-baird removed the grade

[c4-judge]

kirk-baird changed the severity to QA (Quality Assurance)

[0xRizwan]

@kirk-baird

Thanks for the comment.

Admin and setter addresses are completely different and they can not be considered as same. Please refer below code from contract,

File: contracts/rwaOracles/RWADynamicOracle.sol

>>bytes32 public constant SETTER_ROLE = keccak256("SETTER_ROLE");
  bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");

  constructor(
>>  address admin,
>>  address setter,
    address pauser,
    uint256 firstRangeStart,
    uint256 firstRangeEnd,
    uint256 dailyIR,
    uint256 startPrice
  ) {
>>  _grantRole(DEFAULT_ADMIN_ROLE, admin);
    _grantRole(PAUSER_ROLE, pauser);
>>  _grantRole(SETTER_ROLE, setter);

    if (firstRangeStart >= firstRangeEnd) revert InvalidRange();
    uint256 trueStart = (startPrice * ONE) / dailyIR;
    ranges.push(Range(firstRangeStart, firstRangeEnd, dailyIR, trueStart));
  }

It can be seen both admin and setter address have been granted their respective role. It must be noted that the function Natspec and documentation does not mention anything about setter address and its use. This code be redundant too.

This design is for sure against the protocol documentation i.e readme.md in this case. The two references provided in my above comment further proves this issue.

Even if we consider, if the setter and admin addresses were supposed to be same then the roles can be assigned too using _setRoleAdmin() which the contract is already inheriting i.e AccessControlEnumerable.sol, However this is not happening in current implementation.

There is only one admin role in current implementation i.e DEFAULT_ADMIN_ROLE which is used on overrideRange and not on setRange(). Such critical function should not be handled by some third setter address and the documentation also says same and this issue describes same too.

I believe, this is indeed an issue at protocol level and not just a documentation and should be mitigated per the recommendation in report.

[kirk-baird]

The SETTER_ROLE is only used for this function so it looks like a pretty clear design decision to have setter permission level.

This would make it fall under the category of centralisation risks as the admin can change the setter at any time and/or give themself setter permissions.

I still see this issue as QA at best and closer out of scope.

[0xRizwan]

@kirk-baird

I have provided enough burden of proofs as warden for the issue validation. The proofs includes with references with no assumptions.

i respect your judging expertise and if its considered as QA then it could be correct though I tried to explain the issue with the access being given to wrong address.

Thank you!