codehawks-bot commented 1 year ago

DOS and functionality breaking of the Escrow contract due to USDC blacklisted accounts

Severity

Medium Risk

Summary

The possibility of two different problems arising along the blocking of the seller in the USDC contract would make tokens stucked in contract.

Vulnerability Details

Some ERC20 tokens have the ability to block user addresses and contracts, according to which they can disrupt essential functions such as approve, transfer, and transferFrom, which is performed by checking the origin and destination in the mentioned function.

The USDC token, as one of the most popular stable-coins, follow this pattern. As it is mentioned in this article:

When an address is blacklisted, it can no longer receive USDC and all of the USDC controlled by that address is blocked and cannot be transferred on-chain.

Let's dive into the contest. In this competition, there are 5 different parties, 4 of which will definitely be used, which are:

EscrowFactory
Escrow
Buyer
Seller
Arbiter

Considering the first three cases, it seems that the scenario for a possible blockage for Escrow and its Factory is very weak and even if this happens, it will not cause irreversible damage to the protocol as these two cases can be deployed again. In addition, the Buyer, as the initiator of the contract and holder of the token can't make damage to the platform.

But either the last two parties are blocked during the creation of the Escrow contract or even afterward, it will not cause a change in the prevention process, which should be a semantic error.

Let's check the conditions from an ideal point of view so that we proceed according to the principles and with respect to the guidelines and suggestions of the Codehawks site, to say that none of the cases of Escrow, EscrowFactory, Buyer, and the Arbiter, which is the same judge proposed by codehawks, there is no problem as they are completely trusted. With all the respect for the auditors, we cannot have this attitude toward the sellers.

So, in this report, only the effect of blocking sellers before and after the creation of the Escrow contract will be assessed:

Since there is no check or preventive condition in the newEscrow function of the EscrowFactory contract, according to that the transaction will be successful and all the immutable data, including the Seller, will be successfully set, according to which even if the Seller's address is blocked or banned According to the ideal token service, a mistake cannot be made for prevention, and thus in the future, if we want to ignore this part and move forward (not even consider the proposed solutions, except for the one examined along the Escrow contract), we must continue with having this impact on our minds.

Impact

As you know better than I do, there are two possible cases where payment is performed to the Seller:

confirmReceipt()
resolveDespute(uint256 buyerAward)

In the first case when the Seller's address becomes blacklisted in the token contract, thus this function would revert. In this case, the initiateDispute function can be used to solve this problem (considering that the whole topic of this discussion may happen very rarely, so it is reasonable), and then the Arbiter by placing the maximum acceptable value for buyerAward in the resolveDispute function, can prevent the execution of the following condition:

  if (tokenBalance > 0) {
    i_tokenContract.safeTransfer(i_seller, tokenBalance);
  }

And with this, without creating any apparent problem, if there is an arbiter fee, part of the token will be returned to the Buyer, and if the arbiter fee is 0, all of it.

But during this transaction, a front-running problem may arise, and if the front-running is not performed and the transaction is completed, another problem of satisfaction (deduction of the Buyer's token balance) may arise. Thus, we have two major problems here which we will discuss in the preceding:

Satisfaction Issue ( ✅ Tx ):

Since the arbiterFee is a fixed amount and if this problem occurs, it must be paid if there is any amount of it, and this number can even be a relatively large number close to the price, it can cause the Buyer dissatisfaction.

Front-running Issue ( 🌀 Tx ):

This case is actually not a type of permanent disturbance and it can only temporarily disturb the transactions related to resolveDispute(), in such a way that every time the Arbiter wants to save the transaction by maximizing the buyerAward, Someone (it could be the Seller with another address or just another Person) can put more gas than Arbiter and sending only 1 wei of the corresponding ideal token to the Escrow address will cause the Arbiter transaction to fail.

Tools Used

Manual Review, VsCode

Recommendations

I suggest that instead of using buyerAward to return the Buyer token, use sellerAward, which reduces the complexity of the new code of this function and also you can replace the following function:

    function resolveDispute(uint256 sellerAward)
        external
        onlyArbiter
        nonReentrant
        inState(State.Disputed)
    {
        uint256 tokenBalance = i_tokenContract.balanceOf(address(this));
        uint256 totalFee = sellerAward + i_arbiterFee;
        if (totalFee > tokenBalance) {
            revert Escrow__TotalFeeExceedsBalance(tokenBalance, totalFee);
        }

        bool noArbiterFee;
        if (sellerAward != 0)
            try i_tokenContract.transfer(i_seller, sellerAward) {
            } catch {
                noArbiterFee = true;
            }

        if(!noArbiterFee) {
            s_state = State.Resolved;
            emit Resolved(i_buyer, i_seller);
            i_tokenContract.safeTransfer(i_arbiter, i_arbiterFee);
        } else {
            s_state = State.Returned;
            emit Returned(i_buyer, i_seller);
        }

        tokenBalance = i_tokenContract.balanceOf(address(this));
        if (tokenBalance != 0) {
            i_tokenContract.safeTransfer(i_buyer, tokenBalance);
        }
    }

Also, an event and enum member are added for more clarification.

Note: The cost of this transaction can be settled later between the Arbiter and the Buyer who are both trusted, since its a rare situiation.

itsLokacho commented 1 year ago

Escalate

I'd like to escalate this issue as I think it considers different scenarios and describes the issue more comprehensively. The vulnerability here arises from the fact that the USDC contract, which sponsors choose often, has a blacklisting feature in which an address may become banned consequently. Though this report is not just blacklisting a party, it describes how this misbehavior can affect the system in a more general way. The reasons for this escalation is listed below:

I think this effect can damage the system in a more serious behavior as the related duplicate issues just underestimate this vulnerability.
There exists another bug vector inside my report which due to its severity and likelihood I've decided to put its label as medium.

Also the below issues are duplicates of this one (I just looked for those has front-run explanations too, i dont know about the accepted one), that i looked and gathered them around:

798

750

705

558

297

197

164

I'd be appreciated if you escalate and revisit my issue.

PatrickAlphaC commented 1 year ago

The root cause is still the same, so to me the argument is why this one should be selected or not. I'm not sure I've been convinced.

Cyfrin / 2023-07-escrow