howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/main/src/access/AccessControlHooks.sol#L812-L825 https://github.com/code-423n4/2024-08-wildcat/blob/main/src/access/AccessControlHooks.sol#L704-L704 https://github.com/code-423n4/2024-08-wildcat/blob/main/src/access/AccessControlHooks.sol#L531-L561

Vulnerability details

AccessControlHooks::onQueueWithdrawal do not check market.isHooked allowing anyone to call the function with arbitrary hooksData

Summary

AccessControlHooks::onQueueWithdrawal has no access control in contrast to all other hooks, making it possible for an attacker to call _tryValidateAccess(status, lender, hooksData) with arbitrary hooksData.

By calling onQueueWithdrawal directly, the attacker has access to _tryValidateAccess which can update a lender status through _writeLenderStatus
Thus, any Role Provider listed in _roleProviders mapping can be called by the attacker to get credentials, even those who are not in the pull provider list and might be there only to grant access to specific priviledged users.
The call is executed by the Hook, meaning the attacker is calling the role provider with arbitrary data with msg.sender = hook
The attacker can gain access to credentials and operate into the market unexpectedly

Vulnerability details

If we follow the call path:

User call onQueueWithdrawal with arbitrary hooksData (containing a selected role provider)
_tryValidateAccess is called
Then _tryValidateAccessInner is called
_handleHooksData is called and will return hasValidCredential == true if _tryValidateCredential return true
_tryValidateCredential is called
And finally the role provider is extracted from the hooksData and called
When the caller succesfully get (unauthorized) valid credentials, he can then deposit, which will this time call _writeLenderStatus with canSetKnownLender == true, setting isKnownLenderOnMarket == true for that market.
Finally, once isKnownLenderOnMarket == true for the user, he can transfer market's tokens as the onTransfer hook is skipped

For this to be possible step (7) is crucial, and require the provider to return valid credential to the caller, which is possible depending on the verification method used by that provider. The issue would not exist with an access control on the hook.

Under in normal circumstances, Lenders have 2 ways to validate their credential:

a Role Provider calls grantRole with the Lender address
If Lender do not have valid credentials or valid lastProvider, by calling a hooked market function, the hook will go through a list of PullProvider to try validate his credentials

Thanks to the missing access control on onQueueWithdrawal, a Lender can ask credential to any Provider that are not part of the list above, which might be special Providers that shouldn't be accessed by anyone, and might grant credential to the caller.

Impact

Unrestricted access to any Provider by a Lender to try validate his credentials. Also quoting from the "Attack Ideas" section of the Readme :

"Consider ways in which lenders can receive a credential for a market without 'correctly' passing through the hook instance specified by the borrower that deployed it."
"Consider ways in which access to market interactions can be maliciously altered to either block or elevate parties outside of the defined flow."

Tools Used

Manual review

Recommended Mitigation Steps

diff --git a/src/access/AccessControlHooks.sol b/src/access/AccessControlHooks.sol
index 1124e5b..bbbabea 100644
--- a/src/access/AccessControlHooks.sol
+++ b/src/access/AccessControlHooks.sol
@@ -816,6 +816,8 @@ contract AccessControlHooks is MarketConstraintHooks {
     MarketState calldata /* state */,
     bytes calldata hooksData
   ) external override {
+   HookedMarket memory market = _hookedMarkets[msg.sender];
+   if (!market.isHooked) revert NotHookedMarket();
    LenderStatus memory status = _lenderStatus[lender];
    if (
        !isKnownLenderOnMarket[lender][msg.sender] && !_tryValidateAccess(status, lender, hooksData)

Assessed type

Invalid Validation

d1ll0n commented 1 month ago

This is not an issue (certainly not a high). The reason the other functions check isHooked is because they are state-changing beyond the permissions update. onTransfer and onDeposit have the ability to mark an account as a known lender, while onQueueWithdrawal can only update a lender's credentials (provided they have a valid credential to be updated / retrieved from a pull provider). The updating of credentials for a particular lender is not intended to be restricted in any way.

3docSec commented 1 month ago

Interesting finding.

I will go with L because, while the path is viable, it is so under the condition that the providerAddress provided in the hooks data was previously stored in the _roleProviders mapping, so the caller can't avoid passing by a trusted gatekeeper for their call:

File: AccessControlHooks.sol
534:     RoleProvider provider = _roleProviders[providerAddress];
535:     if (provider.isNull()) return false;