code-423n4 / 2023-10-wildcat-findings

14 stars 10 forks source link

A sanctionned lender can transfer his funds before they are escrowed by `nukeFromOrbit()`. #460

Closed c4-submissions closed 1 year ago

c4-submissions commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketToken.sol#L64-L82 https://github.com/code-423n4/2023-10-wildcat/blob/c5df665f0bc2ca5df6f06938d66494b11e7bdada/src/market/WildcatMarketBase.sol#L163-L187

Vulnerability details

Impact

A lender can transfer his tokens before any potential call to nukeFromOrbit() to avoid an escrow to be created with his funds and keep them even if he is sanctionned. He can make bad operations without fear of his tokens being escrowed.

Proof of Concept

Check this scenario:

1) Bob, a lender, has lent some DAI in the "West Ham Capital" market and has equivalent in WHCDAI. He has two wallets (Wallet1 & Wallet2) of lenders accepted in the market. 2) Bob make a bad action and he is sanctionned by the Chainalysis oracle and now he risks having his funds confiscated by a potential call of WildcatMarketConfig::nukeFromOrbit(). 3) Bob create a bot and monitor any call of nukeFromOrbit() with his address to frontrun it with a WildcatMarketToken::transfer() to transfer his WHCDAI to his second wallet (Wallet2).

Check the _blockAccount() function to better understand:

    function _blockAccount(MarketState memory state, address accountAddress) internal {
        Account memory account = _accounts[accountAddress];
        if (account.approval != AuthRole.Blocked) {
            uint104 scaledBalance = account.scaledBalance;
            account.approval = AuthRole.Blocked;
            emit AuthorizationStatusUpdated(accountAddress, AuthRole.Blocked);

            if (scaledBalance > 0) {
                account.scaledBalance = 0;
                address escrow = IWildcatSanctionsSentinel(sentinel).createEscrow(
                    accountAddress,
                    borrower,
                    address(this)
                );
                emit Transfer(accountAddress, escrow, state.normalizeAmount(scaledBalance));
                _accounts[escrow].scaledBalance += scaledBalance;
                emit SanctionedAccountAssetsSentToEscrow(
                    accountAddress,
                    escrow,
                    state.normalizeAmount(scaledBalance)
                );
            }
            _accounts[accountAddress] = account;
        }
    }

I don't know the exact scenarios which a lenders can be sanctionned by the Chainalysis oracle. But it's a possible problematic operation because if a lender have two registered wallets on the market (consciously or unconsciously for the borrower). He can potentially try to make bad operations with one address without fear of being sanctionned by transfering his claimable power to his second address.

Tools Used

Manual Review

Recommended Mitigation Steps

Add a verification step into the custom transfer() and transferFrom() functions in WildcatMarketToken to verify if a lender is sanctionned and prevent him to transfer his tokens when it's the case.

Assessed type

Context

c4-pre-sort commented 1 year ago

minhquanym marked the issue as duplicate of #54

c4-judge commented 1 year ago

MarioPoneder marked the issue as satisfactory