Block-list ERC20s would lead to unwanted functionality

[hats-bug-reporter[bot]]

Github username: @PlamenTSV Twitter username: @p_tsanev Submission hash (on-chain): 0xf930b1b718a0813dd151c796cfebc2e72c86a78c2a6f8f26fbe6fa98fe278d92 Severity: low

Description: Description\ Several function serving as fallbacks like onSendAssetSuccess, onSendAssetFailure etc. by design should never revert and their functionality makes sure they do so. A problem occurs since the failure callback imploys a direct token transfer of escrow tokens which might have the fallback address be block-listed

Attack Scenario\ The most popular such token is USDC. A user sends some USDC assets and defines his own address as the fallback one for the escrow. For some reason his asset sending fails and the contract attempts to return the tokens and emit an event to catch the failure. If the address provided got blocklisted prior to the failed send action, the function would revert, contrary to intended design

Attachments

1. Proof of Concept (PoC) File

2. Revised Code File (Optional)

   In such a scenario it would be best to allow the fallback address to be change-able by itself. If the fallback address got blacklisted, provide another one and send the tokens there.

[reednaa]

Thanks. I can't assign you anything yet but at least low severity.

[reednaa]

Current fix: Convert the safeTransfer from a safe to unsafe (direct transfer).

[reednaa]

We have decided to classify this issue as Low. Our decision is based on the following arguments:

• This issue applies to very few tokens, however, it is tokens that the average user would expect to work with the application.
• This found issue clearly violates the given comments.
• The issue could cause further problems for liquidity providers.

According to these arguments, the issue is low. Internally, we have deemed the proposed fix bad. Instead, it will be implemented as a low level call to transfer. We have yet to figure out how to protect against OOG within the low level call.

[PlamenTSV]

I believe, also based on the same arguments you list, this issue should be medium severity. The most popular stable coins USDC and USDT employ this behavior and there is no doubt they will be one of the first and most widely-used tokens with your protocol. Also consider the fact that non-stable coins tend to greatly change their behavior between chains, there are upgradable tokens that can always implement such functionality as well.

A better fix I believe it to use a pull functionality over the current pull, allowing the user to specify which address should receive the tokens in a separate call on his own. This way you will avoid introducing more problems with low level calls (unless contract size is a problem, but this is 1 short function).

This issue is a historical medium, the 2 examples are the most widely used tokens in the eco-system. Upon request I can list links to a number of past(less than a year ago) issues that solidify the severity here.

I will respect any further decision you make.

Edit: PAX, BUSD, TUSD, general stable coins across chains

[reednaa]

Please provide arguments for why it would be medium beyond the impact it would immediately have on the blacklisted user.

[PlamenTSV]

The freeing of the escow, which is the point of revert, is the last step of the acknowledgement relay. This means that the transaction chain would fail at the very end, denying the relayer his reward and wasting his gas. Any further attempt to relay the same ack would also fail.

[reednaa]

Assuming you have the right proof, acking packages will in general not revert: https://github.com/catalystdao/GeneralisedIncentives/blob/2448d77e412216283ed75d8c3cbaa1270657f7b5/src/IncentivizedMessageEscrow.sol#L384

If you can build a contract such that you cannot ack the package => denying the delivery relayer their reward it would be a medium issue in itself.

[PlamenTSV]

Please provide arguments for why it would be medium beyond the impact it would immediately have on the blacklisted user.

Also it is impact only on the immediate user, but there is no other recovery mechanism so these are undeniably stuck and lost funds. Impacts as bracketed by the provided medium(this is a long-term freezing of user funds which is classified as high, but it's the likelihood that is low): This was given to us here https://hatsfinance.medium.com/catalyst-rewards-up-to-64k-in-usdc-858dab016972

[reednaa]

Notice the fix: https://github.com/catalystdao/catalyst/pull/77

It unfreezes the user's funds by sending them to the vault. The issue is a difference between what actually happens and what was supposed to happen: Ack functions will sometimes fail while it was never supposed to fail.

The fix will still lead to lost user funds, however, it is much clear that the code is working as intended. Note that the difference between blacklisted and transfer fails is almost meaningless.

My comment directly refers to this differentiation which you havn't argued against.

[PlamenTSV]

Yes but the ack function that fails would always fail for that given user. You meet a revert that was never expected to be there and user funds really are frozen. If the contract fails at the ack, then that means the funds on the source were already burned. It covers the points of medium severity: freezing(not really short-term because I do not see how allowing the transfer to fail would protect the user) and loss of funds(as mentioned above)

[reednaa]

I don't understand that point. You repeat what I say is how it supposed to be. You do not argue against my point that in those cases, loss of funds is intended.

[PlamenTSV]

I am pointing out you are confirming that loss of funds will occur for the user. These losses and the short-term DoS that this issue causes are why I am debating for medium severity, based on impacts we were provided to work with. Your fix avoids the ack blockage but the user still loses/freezes his funds, instead of providing him with the option to claim to a different non-blocked address.

[reednaa]

Correct. If the funds were sent (somehow) to the user, they would still be lost as that they are locked.

Given that in both cases, the result is that the user loses access to their funds, the loss of funds must be seen as intended.