c4-submissions commented 11 months ago

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/main/src/market/WildcatMarket.sol#L42

Vulnerability details

Impact

The existing system has been carefully designed to account for OFAC sanctions. In the event that an individual is sanctioned by OFAC and either the nukeFromOrbit() function is triggered or the lender attempts to withdraw funds, their funds are redirected to an escrow. The rationale behind this approach is to safeguard other lenders' funds in the market, preventing contamination that could lead to further sanctions for lenders and borrowers.

The primary issue arises when a lender is sanctioned by OFAC, and one of the following two scenarios occurs (assuming nukeFromOrbit() was not called with the lender's address):

The lender has never had funds in the market.
The lender does not attempt to withdraw funds following the sanction.

In both of these cases, the lender retains the ability to deposit money into the market if they have been authorized by the borrower. This has the effect of tainting all funds in the market, whether due to an accidental interaction or malicious intent, where a lender purposely interacts with a sanctioned address to contaminate their own address and subsequently deposits negligible amounts into a market.

Proof of Concept

The provided gist illustrates the two distinct scenarios mentioned above:

In the first test case, a lender who has already been approved but has never interacted with the market gets sanctioned by OFAC. Subsequently, the lender interacts with the market, contaminating the other lenders' deposits.
In the second test case, a lender has already deposited funds into the contract when they become sanctioned. The lender refrains from attempting to withdraw their funds and deposits another token, thereby contaminating all other lenders with them, as they are unable to retrieve their tokens.

To test this proof of concept, it can be easily added to the test/market folder and executed using the forge test command.

Tools Used

Manual Review

Recommended Mitigation Steps

This issue can be promptly mitigated with a straightforward adjustment. To address this problem, an additional check should be introduced in the deposit functionality. This check will verify whether a lender is sanctioned based on the Chainalysis oracle, and if so, it will revert the transaction when the lender attempts to deposit. An adapted deposit functionality could be structured as follows:

function depositUpTo(
uint256 amount
) public virtual nonReentrant returns (uint256 /* actualAmount */) {
    // Get current state
    MarketState memory state = _getUpdatedState();

    if (state.isClosed) {
    revert DepositToClosedMarket();
    }

    if (sentinel.isSanctioned(borrower, msg.sender)) {
    revert DepositFromSanctionedAddress();
    }

    ...
}

This implementation would also necessitate the addition of a new custom error, namely DepositFromSanctionedAddress.

Assessed type

Other

c4-pre-sort commented 11 months ago

minhquanym marked the issue as duplicate of #326

c4-judge commented 10 months ago

MarioPoneder changed the severity to QA (Quality Assurance)