c4-bot-10 commented 1 month ago

Lines of code

https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarket.sol#L104-L108 https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarket.sol#L117-L120 https://github.com/code-423n4/2024-08-wildcat/blob/fe746cc0fbedc4447a981a50e6ba4c95f98b9fe1/src/market/WildcatMarket.sol#L146-L166

Vulnerability details

Impact

Lenders lose funds and access to deposits

Vulnerability Details

The only function which checks whether the market's borrower is sanctioned is the borrow() function, leaving the rest of the market's functionality operational, such as deposits.

However, deposits should also contain a check because it is very liekly that a sancioned borrower is not going to repay the debts that they owe into the market.

Therefore there would be three impacts on any lenders who deposit after the borrower is sanctioned:

The user loses access to their funds until they can withdraw them. In a fixed term market this could be up to 2 years if the fixed term is set to the maximum and the withdrawal batch duration is set to maximum
The user loses the interest they were expecting to receive for locking their funds away
The user will end up paying off the debts of the borrower; funding other users withdrawals

  function borrow(uint256 amount) external onlyBorrower nonReentrant sphereXGuardExternal {
    // Check if the borrower is flagged as a sanctioned entity on Chainalysis.
    // Uses `isFlaggedByChainalysis` instead of `isSanctioned` to prevent the borrower
    // overriding their sanction status.
    if (_isFlaggedByChainalysis(borrower)) {
      revert_BorrowWhileSanctioned();
    }

    MarketState memory state = _getUpdatedState();
    if (state.isClosed) revert_BorrowFromClosedMarket();

    uint256 borrowable = state.borrowableAssets(totalAssets());
    if (amount > borrowable) revert_BorrowAmountTooHigh();

    // Execute borrow hook if enabled
    hooks.onBorrow(amount, state);

    asset.safeTransfer(msg.sender, amount);
    _writeState(state);
    emit_Borrow(amount);
  }

POC

There is a market at 10% on a 1 year fixed term & 1 month withdrawal
Bob wants to deposit 10e18 asset in a market
Unknown to Bob, the borrower gets sanctioned
Bob deposits his funds without restriction
Bob has to wait 12 months to queue a withdrawal to get money back after 13 months
Bob's funds are used to fund the withdrawals of other users. Due to the pro-rata withdrawal system Bob loses a lot of money

Tools Used

Manual Review Foundry Testing

Recommendations

Add the same check in borrow() to the deposit functions and prevent users depositing if borrower is sanctioned.

Assessed type

Access Control

laurenceday commented 1 month ago

This is not a bug in the protocol itself - the issue here is a strictly off-chain legal aspect rather than in code.

We're interested in making sure that a sanctioned lender can't contaminate the pool for everyone else in a gray-money attack (although they could just maliciously transfer ERC-20 assets and we can't stop that). We also don't want a borrower to be able to pull the rip-cord and flee with all excess reserves. That's why those checks are in place there.

Imposing the same check for deposits is - in our opinion - a waste of gas given how unlikely it is that a borrower address (likely newly created specifically to interact with Wildcat) is going to get tagged by the Chainalysis oracle.

This is the same reason that repay doesn't have a sanctions check on it even though the only party that would reasonably want to use it is the borrower - if they're sanctioned, they probably already know about it, and if they wanted to taint the pool anyway, we couldn't stop it given that they could just transfer directly.

Unsure how to handle the judging of the severity here (in my opinion this is a useful QA, with the remedy being that we block deposits on the frontend if the borrower address is flagged by the Chainalysis oracle), so I'm disputing with a light touch.

3docSec commented 1 month ago

The borrower being sanctioned is public domain, and a lender providing funds to their markets would comfortably fit the "user error" scenario - that's why I am leaving this as L

c4-judge commented 1 month ago

3docSec changed the severity to QA (Quality Assurance)