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

3 stars 1 forks source link

WildcatSanctionsSentinel:: createEscrow allows unregistered market to used to create escrow #65

Closed howlbot-integration[bot] closed 1 month ago

howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/WildcatSanctionsSentinel.sol#L121

Vulnerability details

In WildcatSanctionsSentinel.sol, the createEscrow() function has two issues :

Impact

In WildcatArchController.sol , the isRegisteredMarket function is defined and its used to check if a market is registered in the wildcard system and the removeMarket( ) function which is also defined , is used to remove a market.

However , there is an oversight which allows unregistered and removed markets to to be used by borrowers to create escrow.

This is problematic as malicious market will still be able to interact with the market in various ways.

Proof of Concept

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/WildcatSanctionsSentinel.sol#L121

Tools Used

Manual Review

Recommended Mitigation Steps

  function createEscrow(
    address borrower,
    address account,
    address asset
  ) public override returns (address escrowContract) {
+   if (!IWildcatArchController(archController).isRegisteredMarket(msg.sender) && !IWildcatArchController(archController).isRemovedMarket(msg.sender)) {
      revert NotRegisteredMarket();
    }

Assessed type

Other

d1ll0n commented 2 months ago

There is currently no check that msg.sender is a registered market

This isn't an oversight, there's just no value to performing this check. If someone wants to create their own escrow contract that they're able to unlock, that doesn't affect the protocol in any way. They'd still have to transfer tokens to it.

Does not account for markets that are removed using the removeMarket() function.

Markets being removed just means they won't show up on our UI, it doesn't and shouldn't impact their ability to operate as they normally would on-chain.

Love the confidence putting this as a high though

3docSec commented 1 month ago

The finding is inconsequential; the warden is warmly invited to reflect on this possibility when reporting an impact as vague as:

This is problematic as malicious market will still be able to interact with the market in various ways.

c4-judge commented 1 month ago

3docSec marked the issue as unsatisfactory: Insufficient proof