User can not receive output tokens when the address is blacklisted in case of USDC/USDT

[hats-bug-reporter[bot]]

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

Description: Description\

In Portal.sol, convert() is used to handle the arbitrage conversion of tokens inside the contract for PSM tokens. It means that PSM_ADDRESS will always be be the input hardcoded token address and whatever token address i.e output token that we want to receive after the the conversion.

File: contracts/Portal.sol

    function convert(address _token, uint256 _minReceived, uint256 _deadline) external nonReentrant {
        /// @dev Require that the output token is a valid address and not the input or stake token (PSM / HLP)
        if(_token == PSM_ADDRESS) {revert InvalidToken();}
        if(_token == PRINCIPAL_TOKEN_ADDRESS) {revert InvalidToken();}
        if(_token == address(0)) {revert InvalidToken();}

        /// @dev Require that the deadline has not expired
        if(_deadline < block.timestamp) {revert DeadlineExpired();}

        /// @dev Check if sufficient output token is available in the contract for frontrun protection
        uint256 contractBalance = IERC20(_token).balanceOf(address(this));
        if(contractBalance < _minReceived) {revert InvalidOutput();}
        if(contractBalance == 0)  {revert InvalidOutput();}

        /// @dev Transfer the input (PSM) token from the user to the contract
        IERC20(PSM_ADDRESS).safeTransferFrom(msg.sender, address(this), AMOUNT_TO_CONVERT); 

        /// @dev Update the funding reward pool balance and the tracker of collected rewards
        if (bToken.totalSupply() > 0 && fundingRewardsCollected < fundingMaxRewards) {
            uint256 newRewards = (FUNDING_REWARD_SHARE * AMOUNT_TO_CONVERT) / 100;
            fundingRewardPool += newRewards;
            fundingRewardsCollected += newRewards;
        }

        /// @dev Transfer the output token from the contract to the user
>>        @audit // function will always fail in case msg.sender is blacklisted by  USDC/USDT if the output token is USDC/USDT
>>        IERC20(_token).safeTransfer(msg.sender, contractBalance);
    }

The issue here is with the output token(_token) like USDC, USDT.

USDC tokens have a blacklist() function which is used to blacklist any address by USDC admin. This can be checked here-#2

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

It must be noted that USDC and USDT are widely used tokens in the crypto market.

Attachments

1. Proof of Concept (PoC) File

Consider a scenario,

1) User wants to convert his PSM tokens to USDC/USDT tokens by calling convert() 2) Amount of tokens to be converted i.e AMOUNT_TO_CONVERT is hardcoded as the user is only allowed to sweep entire balance for respective output tokens in one go. 3) The user address i,e msg.sender is found to be blacklisted by USDC/USDT then the transactions will always revert. 4) While transferring the output token in case of USDC/USDT,

        /// @dev Transfer the output token from the contract to the user
        IERC20(_token).safeTransfer(msg.sender, contractBalance);

USDC/USDT contracts always check whether the calling address i.e msg.sender is blacklisted or not.

5) If this scenario happens which is very much likely the user won't be able to convert his tokens to USDC/USDT. It will break the one of the important functionality of contract.

This issue is identified as Medium severity as it falls in below category:

Attacks that make essential functionality of the contracts temporarily unusable or inaccessible.
Short-term freezing of user funds.

2. Revised Code File (Optional)

Recommend to allow the recipient address as function argument and allow user to transfer the output tokens to his desired address.

-    function convert(address _token, uint256 _minReceived, uint256 _deadline) external nonReentrant {
+    function convert(address recipient, address _token, uint256 _minReceived, uint256 _deadline) external nonReentrant {
+     require(recipient != address(0), "invalid address");
+     require(recipient != address(this), "invalid address");
        /// @dev Require that the output token is a valid address and not the input or stake token (PSM / HLP)
        if(_token == PSM_ADDRESS) {revert InvalidToken();}
        if(_token == PRINCIPAL_TOKEN_ADDRESS) {revert InvalidToken();}
        if(_token == address(0)) {revert InvalidToken();}

        /// @dev Require that the deadline has not expired
        if(_deadline < block.timestamp) {revert DeadlineExpired();}

        /// @dev Check if sufficient output token is available in the contract for frontrun protection
        uint256 contractBalance = IERC20(_token).balanceOf(address(this));
        if(contractBalance < _minReceived) {revert InvalidOutput();}
        if(contractBalance == 0)  {revert InvalidOutput();}

        /// @dev Transfer the input (PSM) token from the user to the contract
        IERC20(PSM_ADDRESS).safeTransferFrom(msg.sender, address(this), AMOUNT_TO_CONVERT); 

        /// @dev Update the funding reward pool balance and the tracker of collected rewards
        if (bToken.totalSupply() > 0 && fundingRewardsCollected < fundingMaxRewards) {
            uint256 newRewards = (FUNDING_REWARD_SHARE * AMOUNT_TO_CONVERT) / 100;
            fundingRewardPool += newRewards;
            fundingRewardsCollected += newRewards;
        }

-        /// @dev Transfer the output token from the contract to the user
-        IERC20(_token).safeTransfer(msg.sender, contractBalance);
+        /// @dev Transfer the output token from the contract to the user defined recipient address
+        IERC20(_token).safeTransfer(recipient, contractBalance);
    }

[PossumLabsCrypto]

Thank you for the feedback!

However, we have to dismiss this finding, because this is a latent issue for the entirety of DeFi and therefore common knowledge. We agree that usage of permissioned stable coins should be minimized but this is in the hands of the users to decide.

In your constructed example, the blacklisted caller can simply call the function from a different address. A much larger issue could happen if the Portal itself becomes blacklisted as it would break the functionality if the reward token is USDC/USDT - as is the case with the HLP Portal. However, there is nothing we can do about it.

We take your feedback as a feature request (send received tokens to a specific address) but it is not a vulnerability and the contract works as intended.

[0xRizwan]

@PossumLabsCrypto

Thanks for the comment.

The issue should be considered here. convert() is not converting any arbitrary amount to desired output token. It is taking AMOUNT_TO_CONVERT which is said to be 100000 * 1e18, i.e. 100k tokens.. This is big amount and and if the user would like to convert to USDC/USDT tokens which is most likely as these are most used tokens in crypto then the above explained issues may happen. It is true that we can not predict which address will be blacklisted by USDC/USDT but a future problem from happening could be solved by the given recommendation.

Adding a recipient address is not mere feature but its protecting the user from permanent function failure.

Such issues happening due to blacklisted feature of these tokens has been judged as High/Medium at sherlock and other platforms.

I politely request for the consideration of this issue.

[PossumLabsCrypto]

Your request is well received. However, I suspect there is a misunderstanding.

The 100k tokens you mentioned are PSM tokens that have to be used in convert(). If the user in question was blacklisted by USDC/USDT, he could send PSM tokens to a different wallet and call the convert() from there without a problem.

Hence, accomodating for this in our smart contract is optional, i.e. regarded a feature request.

The proposed solution does not prevent or solve for the Portal itself being blacklisted which is the more severe risk.

(This risk is already partly mitigated by allowing the convert() function to target any token inside the Portal which accomodates a change in reward tokens by the underlying protocol - for example when the blacklisting practices of USDC/USDT become unnacceptable. Though, this is at the discretion of the underlying protocol that we build on.)

[0xRizwan]

@PossumLabsCrypto

I understand your point. Since any address got blacklisted by USDC/USDT will only be known by transaction failure. The portal address has its specific use in protocol but the user can use his address for multiple things. I thought the user should be allowed to pass his desired recipient address and instead of doing multiple transaction for a same task, he could be able to do here in one transaction. imo low severity is still justified here

[PossumLabsCrypto]

I thought the user should be allowed to pass his desired recipient address and instead of doing multiple transaction for a same task

You thinking that the user "should be able to do XYZ to have better UX" is the definition of a feature request. :)

Btw, I don´t debate that this is a useful feature. In fact, we do have this option in mintPortalEnergyToken() and burnPortalEnergyToken() already. However, when we talk about vulnerabilities, it simply does not fit the definition as everything works as intended. Thanks for understanding. 🤝