code-423n4 / 2024-08-wildcat-findings

3 stars 1 forks source link

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

Open howlbot-integration[bot] opened 1 month ago

howlbot-integration[bot] commented 1 month ago

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:

  /**
   * @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

d1ll0n commented 1 month ago

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 commented 1 month ago

There are a few factors to be considered:

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 commented 1 month ago

3docSec marked the issue as selected for report

c4-judge commented 1 month ago

3docSec marked the issue as satisfactory