The down of Chainalysis's SanctionsList leads to the malfunctioning of Wildcat's functions

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L14 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L42 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsEscrow.sol#L26 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketConfig.sol#L75 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketConfig.sol#L94 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketWithdrawals.sol#L164

Vulnerability details

The chainalysisSanctionsList parameter of the WildcatSanctionsSentinel contract is an immutable variable. It will be unchangeable if Chainalysis's SanctionsList contract is down, affecting several essential functions in the Wildcat protocol to malfunction.

Proof of Concept

The vulnerability flagged in this report is not about any centralization or trust risks of Chainalysis; it is only about the risks from human errors or management mistakes. However, relying on the third party's security (Chainalysis) should not be the best security idea of the Wildcat protocol.

In the WildcatSanctionsSentinel contract, the chainalysisSanctionsList is declared immutable, pointing to Chainalysis's SanctionsList contract. Therefore, no one can update it if necessary after the Sentinel contract is deployed.

The chainalysisSanctionsList parameter is employed in the critical WildcatSanctionsSentinel::isSanctioned() to query the sanction status of all lenders in every Market contract throughout the protocol.

I noticed that Chainalysis's SanctionsList contract implements the critical single-step functions: renounceOwnership() and transferOwnership(). If one of them is somehow executed incorrectly, that could brick the SanctionsList contract eternally.

Consequently, the Wildcat protocol's functions will be directly damaged since no one can update the chainalysisSanctionsList parameter.

    contract WildcatSanctionsSentinel is IWildcatSanctionsSentinel {
      ...

@>    address public immutable override chainalysisSanctionsList;

      ...

      /**
      * @dev Returns boolean indicating whether `account` is sanctioned
      *      on Chainalysis and that status has not been overridden by
      *      `borrower`.
      */
      function isSanctioned(address borrower, address account) public view override returns (bool) {
        return
          !sanctionOverrides[borrower][account] &&
@>        IChainalysisSanctionsList(chainalysisSanctionsList).isSanctioned(account);
      }

      ...
    }

• https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L14

• https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/WildcatSanctionsSentinel.sol#L42

Impact

The vulnerability flagged in this report is not about any centralization or trust risks of Chainalysis; it is only about the risks from human errors or management mistakes. However, relying on the third party's security (Chainalysis) should not be the best security idea of the Wildcat protocol.

The following lists essential functions that require the oracle data sourced from Chainalysis's SanctionsList contract.

1. WildcatSanctionsEscrow contract

   1. WildcatSanctionsEscrow::canReleaseEscrow()

2. WildcatMarket contract

   1. WildcatMarketConfig::nukeFromOrbit()
   2. WildcatMarketConfig::stunningReversal()
   3. WildcatMarketWithdrawals::executeWithdrawal()

   If Chainalysis's SanctionsList contract is down, the above functions could malfunction. For instance, no one, including the borrower and the Wildcat protocol owner, can block any sanctioned lenders from interacting with their markets.

   Moreover, the bricking of the SanctionsList will affect all deployed Market contracts, MarketController contracts, and MarketControllerFactory contracts as they point to the singleton Sentinel contract.

Tools Used

Manual Review

Recommended Mitigation Steps

The chainalysisSanctionsList parameter in the WildcatSanctionsSentinel contract should be a state variable allowing authorized users (e.g., the protocol owner) to update it when necessary.

Since the Sentinel is a singleton contract, updating the chainalysisSanctionsList parameter at the Sentinel would be effective for all deployed Market contracts, MarketController contracts, and MarketControllerFactory contracts discussed previously.

Assessed type

Oracle

[c4-pre-sort]

minhquanym marked the issue as primary issue

[c4-pre-sort]

minhquanym marked the issue as sufficient quality report

[c4-sponsor]

d1ll0n (sponsor) acknowledged

[c4-sponsor]

d1ll0n marked the issue as disagree with severity

[d1ll0n]

I think this could be a good QA finding but I wouldn't consider it medium severity, or even fixable. Allowing anyone to change the sanctions list address would only replace this issue with a much worse one, namely that the Wildcat team would be able to block lenders from markets.

Also worth noting that the sanctions list can not be paused as suggested in #138 - the effect of this issue (ignoring the possibility of a malicious owner - only considering the key being lost) would just be that the contract stops adding new sanctioned addresses.

[serial-coder]

I'm new to the +Backstage, and this is my first +Backstage comment. I'm not sure if I'm allowed to reply to the sponsor's comment for now. If not, please do not suspend my +Backstage privileges.

First Point

As the sponsor said:

Allowing anyone to change the sanctions list address would only replace this issue with a much worse one, namely that the Wildcat team would be able to block lenders from markets.

Please let me clarify the sponsor's comment with this excerpted part from the Recommended Mitigation Steps section:

The chainalysisSanctionsList parameter in the WildcatSanctionsSentinel contract should be a state variable allowing authorized users (e.g., the protocol owner) to update it when necessary.

Specifically, the report suggested that only authorized staff (e.g., the Wildcat protocol owner/admin) should be able to update the chainalysisSanctionsList parameter, not any unauthorized user.

Second Point

I raised this issue because the SanctionsList contract implements the critical single-step functions: renounceOwnership() and transferOwnership(). If one of them is executed incorrectly (by mistakes or human errors), that could brick the SanctionsList contract eternally.

As the sponsor said:

The Wildcat team would be able to block lenders from markets.

I'm not arguing that.

However, if the SanctionsList cannot add or remove sanctioned addresses, this can directly affect Wildcat's essential functions relying on it, including WildcatSanctionsEscrow::canReleaseEscrow(), WildcatMarketConfig::nukeFromOrbit(), WildcatMarketConfig::stunningReversal(), WildcatMarketWithdrawals::executeWithdrawal(), and their dependency functions.

Of course, the Wildcat team might be able to block lenders from markets manually, but the incident will break the protocol functionality forever. Moreover, this also reduces the transparency and/or decentralization of managing sanctioned lenders.

For this reason, relying on the third party's security (Chainalysis) should not be the best security idea of the Wildcat protocol.

Third point

My suggested solution can fully fix the vulnerability without leading to other issues.

Please re-consider this excerpted part from the Recommended Mitigation Steps section thoroughly:

The chainalysisSanctionsList parameter in the WildcatSanctionsSentinel contract should be a state variable allowing authorized users (e.g., the protocol owner) to update it when necessary.

Since the Sentinel is a singleton contract, updating the chainalysisSanctionsList parameter at the Sentinel would be effective for all deployed Market contracts, MarketController contracts, and MarketControllerFactory contracts discussed previously.

[MarioPoneder]

It`s worth to mention that the Chainalysis: Sanctions Oracle's owner is a simple walllet, not a multisig, not a timelock. Therefore, the concern that it could be compromised is valid. (But this external dependency is not in scope of this contest.)

On the other hand, the proposed solution that only authorized staff (e.g., the Wildcat protocol owner/admin) should be able to set a new sanctions list address adds a second point of failure. Therefore, not a viable solution.

As this is not a bug of the protocol itself and cannot be circumvented easily in a satisfactory way, QA seems most appropriate.

@serial-coder I want to assure you that your comment before post-judging QA did not have any impact on my assessment of severity.
As a fresh backstage warden, mistakes can happen. All the best for your journey on C4.

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-a