User will be unable to withdraw tokens if his address is blacklisted in case of USDC/USDT

[hats-bug-reporter[bot]]

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0x009ca56920ee3c664dfdfd9539f4beaa8e282d6002a26f84b350df4cc1e99766 Severity: high

Description: Description\

WiseLending contract allows the users to deposit crypto assets which they intend to hold long-term or short term into a lending platform. The motive of users is to earn passive income from borrowers.

WiseLending contract has deposit functions which users can call to deposit their crypto assets. The contracts refers these crypto assets as _poolToken which consists of USDT,USDC,DAI, etc. Upon seeing this, different types of _poolToken supported by contract can be understood.

Similarly, WiseLending contract has withdraw functions which can be used by users to withdraw the deposited ERC20 tokens.

To understand better, take withdrawExactAmount() from WiseLending contract. The users wants to withdraw token can calls this function and safeTransfer method will transfer back their tokens to msg.sender i.e calling user.

    function withdrawExactAmount(
        uint256 _nftId,
        address _poolToken,
        uint256 _withdrawAmount
    )
        external
        syncPool(_poolToken)
        healthStateCheck(_nftId)
        returns (uint256)
    {

     . . . some code 

        _safeTransfer(
            _poolToken,
            msg.sender,             
            _withdrawAmount
        );

        return withdrawShares;
    }

The issue here is with the tokens like USDC, USDT which are supported as pool token by platform. USDC tokens have a blacklist() function which is used to blacklist any address by USDC admin. This can be checked here

Similarly, USDT tokens have a addToBlockedList() function which is used to blacklist any address by USDT admin. This can be checked here

The issue here is, the user who had deposited the ERC20 tokens in WiseLending contract can get blacklisted by USDC/USDT after depositing of tokens. If they try withdrawing after few months, the transaction will be reverted as the user address is blacklisted by USDC/USDT.

To get the issue better, let's see below scenario with USDC token,

1) User calls deposit functions on WiseLending contract with one of the poolToken as USDC/USDT tokens to deposit his tokens which are then transferred to address(this) i.e WiseLending contract.

2) After few months/years, Now user thought to withdraw the poolToken by calling the Withdraw functions on WiseLending contract. The user finds that his address i.e msg.sender is found to be blacklisted by USDC then the transactions fails,

        _safeTransfer(
            _poolToken,
            msg.sender,              @audit // pool token directy transfers  to msg.sender i.e calling user
            _withdrawAmount
        );

It checks the recipient address is blacklisted or not. msg.sender is the recipient in our case.

Below is the transfer method of USDC,

    function transfer(address to, uint256 value)
        external
        override
        whenNotPaused
        notBlacklisted(msg.sender)
        notBlacklisted(to)                            @audit// checks recipient is blacklisted or not
        returns (bool)
    {
        _transfer(msg.sender, to, value);
        return true;
    }

USDC notBlacklisted() modifier,

    modifier notBlacklisted(address _account) {
        require(
            !blacklisted[_account],
            "Blacklistable: account is blacklisted"
        );
        _;
    }

So the withdrawal of tokens will awalys revert and If this scenario happens which is very much likely so the user won't be able to withdraw his assets i.e poolToken deposited to wise lending contract then the function will always revert. It will break the one of the important functionality of contract which will cause complete Dos of withraw to users.

Recommendation\

Recommend to allow the recipient address as function argument in withdraw functions and allow user to transfer the the assets/poolToken to his desired address.

For example:

    function withdrawExactAmount(
        uint256 _nftId,
+      address _recipient,
        address _poolToken,
        uint256 _withdrawAmount
    )
        external
        syncPool(_poolToken)
        healthStateCheck(_nftId)
        returns (uint256)
    {
+     require(recipient != address(0), "invalid address");
+     require(recipient != address(this), "invalid address");

     . . . some code

-        _safeTransfer(
-            _poolToken,
-            msg.sender,
-            _withdrawAmount
-        );

+        _safeTransfer(
+            _poolToken,
+            recipient,                   @audit //Use recipient address instead of msg.sender
+            _withdrawAmount
+        );

     . . . some code
    }

[vm06007]

This is quite unlikely and the blacklisted address can always simply move _nftId to another wallet, changing position ownership of who can withdraw funds.

So, keep in mind that if address was blacklisted even then this account could potentially just transfer NFT from blacklisted wallet to another wallet since ownership of the position is determined by who owns the NFT, so this won't be a problem in the end to bypass.

Also, topic of decentralization is out of scope, with same in mind this is a question on USDC/USDT token contract owner part, you can say they will renounce ownership, they can also block entire protocols by blacklisting protocol addresses.

Also adding _recipient in the withdraw function leads to way more problems, and then USDT/USDC can blacklist where bad-actor will withdraw tokens anyway letting them get-away or blacklisting next address where tokens are withdrawn. Either way this is quite a bad design suggestion from your side without realizing user does not need this functionality as another parameter, user already have such functionality by having ability to move WiseLendingNFT (_nftId) from one wallet to another choosing which wallet can withdraw funds.

This scenario is very unlikely that USDT/USDC will start blacklisting out of no good reason. Even if someone is blacklisted this does not lead to entire protocol being frozen as you suggest by saying It will break the one of the important functionality of contract which will cause complete Dos of withraw to users. All other users will be still able to use protocol, just the person who is blacklisted won't be and I think this is desired scenario since this means its a bad-actor and should be stopped from moving stolen funds around, apprehended eventually and NFT for WiseLending taken in custody.

[vm06007]

@0xRizwan let me know if you disagree on comments above that user can already move _nftId between wallets to avoid being blacklisted.

[0xRizwan]

@vm06007

Thanks for the detailed response. I agree with your explanation.

I think, this issue pertaining to USDC/USDT should be explicitely mention in documentation as the end user is not of aware if this issue happens to him/her. I would still see this issue as low severity due to weird behavior of supported pool tokens, also such issue info is not mentioned in protocol documentation.

Please check if this issue can be made in report for the best interest of users.

[vm06007]

@0xRizwan what issue? I think it is not protocol related, it is USDT/USDC token related and is a subject for anywhere these tokens are used that user needs to know that owning these tokens comes with a risk that user wallets can always be blacklisted regardless if they enter any protocol or not.

[0xRizwan]

@vm06007 Not every user is expected to know about weird or non-standard behavior of tokens like here USDC/USDT. To be honest, most of the auditors don't know about this issue. You have to document it or alert some message at front-end for users using these tokens.

Well, If its not intended to even consider for a low severity then there is no comment from my side.

[vm06007]

@0xRizwan: So you are expecting all protocols to display some message on UI to warn user about blacklist functionality if user has USDT/USDC in their wallet?

Could you point to any protocol that display this for user in their UI as a warning? Maybe it should be displayed on exchanges instead to warn users when they are opting-in buying USDT/USDC that this token has a blacklist function so users know this when they buy it, not after they bought it and not each protocol is responsible to notify users.

In other words it would make more sense to notify users on a gateway (hence exchanges) before user even decides to own such tokens.