Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Role providers can bypass intended restrictions and lower expiry set by other providers #57

Open
howlbot-integration bot opened this issue Sep 20, 2024 · 4 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working M-05 primary issue Highest quality submission among a set of duplicates 🤖_primary AI based primary recommendation 🤖_18_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality

Comments

@howlbot-integration
Copy link

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/access/FixedTermLoanHooks.sol#L413

Vulnerability details

Proof of Concept

If we look at the code comments, we'll see that role providers can update a user's credential only if at least one of the 3 is true:

  • the previous credential's provider is no longer supported, OR
  • the caller is the previous role provider, OR
  • the new expiry is later than the current expiry
  /**
   * @dev Grants a role to an account by updating the account's status.
   *      Can only be called by an approved role provider.
   *
   *      If the account has an existing credential, it can only be updated if:
   *      - the previous credential's provider is no longer supported, OR
   *      - the caller is the previous role provider, OR
   *      - the new expiry is later than the current expiry
   */
  function grantRole(address account, uint32 roleGrantedTimestamp) external {
    RoleProvider callingProvider = _roleProviders[msg.sender];

    if (callingProvider.isNull()) revert ProviderNotFound();

    _grantRole(callingProvider, account, roleGrantedTimestamp);
  }

This means that a role providers should not be able to reduce a credential set by another role provider.

However, this could easily be bypassed by simply splitting the call into 2 separate ones:

  1. First one to set the expiry slightly later than the currently set one. This would set the role provider to the new one.
  2. Second call to reduce the expiry as much as they'd like. Since they're the previous provider they can do that.

Recommended Mitigation Steps

Fix is non-trivial.

Assessed type

Context

@howlbot-integration howlbot-integration bot added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value 🤖_18_group AI based duplicate group recommendation 🤖_primary AI based primary recommendation bug Something isn't working primary issue Highest quality submission among a set of duplicates sufficient quality report This report is of sufficient quality labels Sep 20, 2024
howlbot-integration bot added a commit that referenced this issue Sep 20, 2024
@d1ll0n
Copy link

d1ll0n commented Sep 30, 2024

This is a useful note to be aware of, but I'd categorize it low/informational as role providers are inherently trusted entities. The likelihood and impact of this kind of attack are pretty minimal.

@3docSec
Copy link

3docSec commented Oct 4, 2024

There are a few factors to be considered:

  • it is a valid privilege escalation vector
  • the attacker has to be privileged already
  • the attack can have a direct impact on a lender

While the first two have me on the fence when choosing between M and L severity, the third point is a tiebreaker towards M.
If we stick to the C4 severity categorization, I see a good fit with the Med definition:

the function of the protocol or its availability could be impacted [...] with a hypothetical attack path with stated assumptions

@c4-judge
Copy link
Contributor

c4-judge commented Oct 4, 2024

3docSec marked the issue as selected for report

@c4-judge c4-judge added the selected for report This submission will be included/highlighted in the audit report label Oct 4, 2024
@c4-judge
Copy link
Contributor

c4-judge commented Oct 4, 2024

3docSec marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Oct 4, 2024
@C4-Staff C4-Staff added the M-05 label Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working M-05 primary issue Highest quality submission among a set of duplicates 🤖_primary AI based primary recommendation 🤖_18_group AI based duplicate group recommendation satisfactory satisfies C4 submission criteria; eligible for awards selected for report This submission will be included/highlighted in the audit report sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

4 participants