search

Cyfrin / 2023-07-escrow

16 stars 12 forks source link

High - Funds can be lost if any participant is blacklisted #154

Open codehawks-bot opened 1 year ago

codehawks-bot commented 1 year ago

High - Funds can be lost if any participant is blacklisted

Severity

High Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-07-escrow/blob/fa9f20a273f1e004c1b11985ef0518b2b8ae1b10/src/Escrow.sol#L98

https://github.com/Cyfrin/2023-07-escrow/blob/fa9f20a273f1e004c1b11985ef0518b2b8ae1b10/src/Escrow.sol#L120

https://github.com/Cyfrin/2023-07-escrow/blob/fa9f20a273f1e004c1b11985ef0518b2b8ae1b10/src/Escrow.sol#L123

https://github.com/Cyfrin/2023-07-escrow/blob/fa9f20a273f1e004c1b11985ef0518b2b8ae1b10/src/Escrow.sol#L127

Summary

It is stated in the readme and by Patrick Collins that the protocol will only accept vetted tokens such as USDC, DAI, WETH, etc..

Yet by using these popular tokens such as USDC, there exists a case where the funds for every participant will locked permanently. This is due to the blacklist system which is implemented by USDC and many other popular well-reputed tokens.

in Escrow.sol, if any of the participants is blacklisted (due to illegal activities, using Tornado Cash for privacy reasons, spite by one of the participants), the funds for every participant will be locked permanently.

Vulnerability Details

In the Escrow.sol contract, when an escrow is created, there are two ways to "get the funds out".

  1. If there is no dispute, the buyer calls the function confirmReceipt() which will transfer the entire contract token balance to the seller address.
  2. If there is a dispute, either the buyer or the seller can call initiateDispute() and the arbiter can call the function resolveDispute which will transfer part of the funds to the buyer address, a fee to the arbiter address and finally, the potential remaining funds in the contract to the seller address

If one the participant's addresses that is set to receive funds is blacklisted however, the functions will revert and essentially "brick" the contract, making it impossible to ever recover the funds.

Blacklisting is certainly not uncommon and is used many of the popular token used for payments, such as the stablecoin USDC. An address can get blacklisted for illegal activities, some were blacklisted for just using tools like Tornado Cash to keep their transactions private and it is also perfectly possible for a disgruntled participant to intentionally blacklist his address to block the withdrawal of funds.

Essentially, if any of the addresses involved is blacklisted, none of the participants can receive their funds.

We have included a POC to showcase how it works in all the cases. You can get the POC file in the following gist: https://gist.github.com/TheNaubit/b0cc2e6b4d1ae2bea637d9d89d9b5b19

To run it, has paste the content of the gist inside a file called WithdrawFailBlacklisted.t.sol inside the test folder in the project. Then run them with the following command:

forge test --match-contract WithdrawFailBlacklisted

Affected code:

For reference, there are other contests with similar findings like:

Impact

When this issue happens, all the funds in the contract are locked forever making every participant to lose their funds.

Tools Used

Manual review & Foundry

Recommendations

There are one solution with two parts:

  1. Instead of trying to transfer the funds to each address, store in a state variable how many funds each address can withdraw and then create a withdraw function only callable when the escrow is finished where each participant can withdraw the funds they own. In this case, if any participant is blacklisted, at least the rest will be able to get the rest of the funds.
  2. The second part of the solution is to solve the part of some participant not being able to withdraw if they are blacklisted. A solution would be to implement a function to allow each participant to set another withdrawal address for their funds, so even if they are blacklisted, they can at least withdraw those funds to another address.
PatrickAlphaC commented 1 year ago

Great write up and recommendation.

PatrickAlphaC commented 1 year ago

Likelihood: Medium Impact: High

High is deserved. One could argue the likelihood is low, but I clearly stated that USDC would be an acceptable currency. Great writeup. Great recommendation. Deserved selected.

nevillehuang commented 1 year ago

This should be M severity given the low likelihood. Somebody mentioned the stats in the discord.

https://discord.com/channels/1127263608246636635/1129041993323004044/1146389688651886716

0xmahdirostami commented 1 year ago

Hello, I want to give my opinion, reporting on this one because it is the selected one.

The impact mentioned here is that all the funds in the contract are locked forever making every participant lose their funds.. But remember, the current implementation doesn't work with fees on transfer tokens, so USDC, DAI, WETH, USDT couldn't be used and there is no fund at risk. And if there is no fund at risk, So it shouldn't be high.

talfao commented 1 year ago

The previous comment does not make sense. The issue points out that the assets are at risk if the user is blacklisted. That is common for USDC - what will be one of the whitelisted tokens.

The issue should remain high, as the PatrickAlpha states. This problem in this kind of application is huge as the seller will not get any rewards for his work because of conditions that are influenced by a third party (USDC contract).

The severity, in this case, needs to be checked based on the type of application. I see this as a significant problem for the Escrow contract.

0xmahdirostami commented 1 year ago

I mentioned that as USDC is fees on transfer token, in the first place, it couldn't be used. (So there is no fund at risk). The current implementation doesn't work with fees on transfer tokens.

talfao commented 1 year ago

It is true that USDC is a fee on transfer tokens. However, currently, the fee is set to zero. Still, you are pointing out another possible issue that the protocol does not take into account, but you have to address both of them. Nothing changes that this blacklist issue occurs.

alymurtazamemon commented 1 year ago

I think USDC is the most popular coin and if the team had mentioned that they would not use the fee-on-transfer tokens then most probably they were not talking about USDC.

Blacklisting is a legit issue and due to impact-and-likelihood, it is up to the judges to give H/M.

By the way, I did not submit this issue or similar so I am a neutral judge.

0xdeadbeef0x commented 1 year ago

Escalating this to be low.

A bit on blacklisting: Blacklisting issues are usually deemed invalid in other contest platforms if no harm to the users/protocol. The reason is that usually an address that gets blacklisted will only cause harm to itself.

I remind you that for an actor to be blacklisted he needs to do a malicious action that is picked up by circle and the US government. read more: https://f.hubspotusercontent30.net/hubfs/9304636/PDF/Centre_Blacklisting_Policy_20200512.pdf

The issue claims any of the addresses involved is blacklisted, none of the participants can receive their funds. This is not true - If only one party is blacklisted he cannot receive his funds (which makes sense because he is blacklisted). The other parties can retrieve their funds - lets consider each actor:

I have commented on each test scenario in the POC to explain why the scenario is invalid

  function testExpectRevertInConfirmReceiptIfSellerIsBlacklisted() external blacklistAddress(seller) {
      vm.startPrank(buyer);
      vm.expectRevert(abi.encodeWithSelector(IERC20WithBlacklist.ERC20WithBlacklist__Blacklisted.selector));
      deployedEscrow.confirmReceipt(); 
      vm.stopPrank(); // @ESCALATION: If seller is blacklisted he cannot receive funds (design of blacklist)
  }

  function testExpectRevertInResolveDisputeIfSellerIsBlacklisted() external blacklistAddress(seller) startDispute {
      vm.startPrank(arbiter);
      vm.expectRevert(abi.encodeWithSelector(IERC20WithBlacklist.ERC20WithBlacklist__Blacklisted.selector));
      deployedEscrow.resolveDispute(BUYER_AWARD_ON_DISPUTE);
      vm.stopPrank(); // @ESCALATION: If seller is blacklisted, the remaining of the funds can go back to the buyer by
     // setting BUYER_AWARD_ON_DISPUTE to required funds. The function will not revert 
  }

  function testExpectRevertInResolveDisputeIfBuyerIsBlacklisted() external blacklistAddress(buyer) startDispute {
      vm.startPrank(arbiter);
      vm.expectRevert(abi.encodeWithSelector(IERC20WithBlacklist.ERC20WithBlacklist__Blacklisted.selector));
      deployedEscrow.resolveDispute(BUYER_AWARD_ON_DISPUTE);
      vm.stopPrank(); // @ESCALATION: If buyer is blacklisted, BUYER_AWARD_ON_DISPUTE = 0 will not revert and seller
     // will receive rest of the funds.
  }

  function testExpectRevertInResolveDisputeIfArbiterIsBlacklisted() external blacklistAddress(arbiter) startDispute {
      vm.startPrank(arbiter);
      vm.expectRevert(abi.encodeWithSelector(IERC20WithBlacklist.ERC20WithBlacklist__Blacklisted.selector));
      deployedEscrow.resolveDispute(BUYER_AWARD_ON_DISPUTE); // @ESCALATION: Arbiter is trusted... should not be blacklister. 
// If blacklisted, he won't receive payment so no incentive to solve disputes either way (known issue/no arbiter issue)
      vm.stopPrank(); 
  }

In the unlikely case that the buyer or seller act maliciously and get blacklisted (they should be trusted) the escrow can still perform as planned and no funds should be lost on the innocent party.

To conclude: Lets forget the fact that they are trusted - being blacklisted will not prevent payment to other actors I see this as low impact (no funds lost) and low likelihood (trusted actors need to be blacklisted)

PatrickAlphaC commented 1 year ago

Interesting... Checking

PatrickAlphaC commented 1 year ago

After consulting with the team, and reading the comments, we are moving to medium. The likelihood of this happening is low, but the impact is high, so this should be fixed.