A `borrower` can remove itself from role providers of Hooks

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/access/AccessControlHooks.sol#L249-L259 https://github.com/code-423n4/2024-08-wildcat/blob/main/src/access/FixedTermLoanHooks.sol#L286-L296

Vulnerability details

Impact

Once the borrower is removed from the role providers, it cannot be added again, resulting in the borrower having no way to directly grant roles.

Proof of Concept

When a hook instance of AccessControlHooks‎ or FixedTermLoanHooks‎ is deployed, the deployer(borrower) itself will be added as the default role provider:

  constructor(address _deployer, bytes memory /* args */) IHooks() {
    borrower = _deployer;
    // Allow deployer to grant roles with no expiry
    _roleProviders[_deployer] = encodeRoleProvider(
      type(uint32).max,
      _deployer,
      NotPullProviderIndex
    );
    ...
  }

The borrower of the hook instance can add new role provider or remove existing role provider as their wish. However, when the borrower itself is removed from the role providers, it cannot be added back, as the borrower is unlikely to be a valid IRoleProvider address, and the call IRoleProvider#isPullProvider‎() will always revert:

  function addRoleProvider(address providerAddress, uint32 timeToLive) external onlyBorrower {
    RoleProvider provider = _roleProviders[providerAddress];
    if (provider.isNull()) {
@>    bool isPullProvider = IRoleProvider(providerAddress).isPullProvider();
      // Role providers that are not pull providers have `pullProviderIndex` set to
      // `NotPullProviderIndex` (max uint24) to indicate they do not refresh credentials.
      provider = encodeRoleProvider(
        timeToLive,
        providerAddress,
        isPullProvider ? uint24(_pullProviders.length) : NotPullProviderIndex
      );
      if (isPullProvider) {
        _pullProviders.push(provider);
      }
      emit RoleProviderAdded(providerAddress, timeToLive, provider.pullProviderIndex());
    } else {
      // If provider already exists, the only value that can be updated is the TTL
      provider = provider.setTimeToLive(timeToLive);
      uint24 pullProviderIndex = provider.pullProviderIndex();
      if (pullProviderIndex != NotPullProviderIndex) {
        _pullProviders[pullProviderIndex] = provider;
      } else {}
      emit RoleProviderUpdated(providerAddress, timeToLive, pullProviderIndex);
    }
    // Update the provider in storage
    _roleProviders[providerAddress] = provider;
  }

Copy below codes to AccessControlHooks.t.sol and run forge test --match-test test_addBorrowerAsRoleProvider:

  function test_addBorrowerAsRoleProvider() external {
    //@audit-info borrower(address(this)) is valid role provider
    assertNotEq(RoleProvider.unwrap(hooks.getRoleProvider(address(this))), 0);
    //@audit-info borrower remove itself from role providers
    hooks.removeRoleProvider(address(this));
    assertEq(RoleProvider.unwrap(hooks.getRoleProvider(address(this))), 0);
    //@audit-info this is no way to add borrower itself to role providers again
    vm.expectRevert();
    hooks.addRoleProvider(address(this), type(uint32).max);
  }

Tools Used

Manual review

Recommended Mitigation Steps

borrower should never be removed from role providers:

  function removeRoleProvider(address providerAddress) external onlyBorrower {
+   if (providerAddress == borrower) {
+       revert ProviderNotRemoved();
+   }
    RoleProvider provider = _roleProviders[providerAddress];
    if (provider.isNull()) revert ProviderNotFound();
    // Remove the provider from `_roleProviders`
    _roleProviders[providerAddress] = EmptyRoleProvider;
    emit RoleProviderRemoved(providerAddress, provider.pullProviderIndex());
    // If the provider is a pull provider, remove it from `_pullProviders`
    if (provider.isPullProvider()) {
      _removePullProvider(provider.pullProviderIndex());
    }
  }

Assessed type

Access Control

[laurenceday]

Not a concern. A borrower may desire the ability to remove themselves as a role provider in the event that compliance/legal guidance directs that their access credentials should all be granted through - for example - role providers with an on-chain KYC trail.

[3docSec]

The borrower renounces a privilege they may need later. Looks a pretty clear user error

[c4-judge]

3docSec marked the issue as unsatisfactory: Invalid