Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comment about TUSD in fixMediatorBalance #39

Merged
merged 2 commits into from
Apr 6, 2021
Merged

Comment about TUSD in fixMediatorBalance #39

merged 2 commits into from
Apr 6, 2021

Conversation

k1rill-fedoseev
Copy link
Member

@k1rill-fedoseev k1rill-fedoseev commented Mar 30, 2021

Tokens that have more than one address, through which they can be called, can be stolen when
they are bridged. An example for such a token is TUSD. The attack would work as follows:

  1. The token is already bridged using the first token address. An amount X has been transferred across the bridge.
  2. An attacker bridges the token using another token address. The attacker also bridges X tokens. Now the mediator balance on the native side is X for both token addresses. However, the actual balance, when queried from balanceOf is 2*X for both of them.
  3. The attacker colludes with the administrators, which trigger a call to fixMediatorBalance on the native side and withdraw X amount of tokens.
  4. Then, the attacker can withdraw X tokens, by sending back the bridged tokens.
    Overall, turned X tokens into 2*X tokens, when ignoring fees. The attacker managed to withdraw the full amount of bridged tokens.

@k1rill-fedoseev k1rill-fedoseev self-assigned this Mar 30, 2021
Copy link
Collaborator

@akolotov akolotov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue is even more general: how the system should behave if it was discovered during the very first transfer of the token that the OB contract already owns some tokens. Does it make sense to set a flag that the mediator balance is not zero at the moment of the first token transfer as so this flag will require special handling of the token when fixMediatorBalance is going to be used?

@akolotov akolotov added audit team-april-2021 Related to items found by auditors which we called "Team April 2021" labels Apr 2, 2021
@k1rill-fedoseev
Copy link
Member Author

The issue is even more general: how the system should behave if it was discovered during the very first transfer of the token that the OB contract already owns some tokens. Does it make sense to set a flag that the mediator balance is not zero at the moment of the first token transfer as so this flag will require special handling of the token when fixMediatorBalance is going to be used?

  1. Do you know any other tokens, except TUSD, that have more than 1 address?
  2. What do you mean by "special handling"? Introduce a fixMediatorBalance(address token, address receiver, bool force) function?

@akolotov
Copy link
Collaborator

akolotov commented Apr 6, 2021

  1. Do you know any other tokens, except TUSD, that have more than 1 address?

I haven't known even about the second address of TUSD. But I think that such tokens exist or at least could appear at any moment.

  1. What do you mean by "special handling"? Introduce a fixMediatorBalance(address token, address receiver, bool force) function?

Ok, I thought about this a bit more and it seems that there is no simple way to pay additional attention of admins that the discussed situation happens. E.g. this imbalance will occur every time when users use the second token address, not only when the very first transfer happens. So the only way to notify admins about a suspicious actions is to run an offchain code which will analyse every transfer made through OB to identify if there are several addresses serving the same token.

Don't you mind to change the comment to "Before the call of this his method, it must be carefully investigated how imbalance happens in order to avoid attempt to stole the funds from a token with double addresses (e.g. TUSD accessible at both 0x8dd5fbCe2F6a956C3022bA3663759011Dd51e73E and 0x0000000000085d4780B73119b644AE5ecd22b376)"

@akolotov akolotov merged commit f2d9028 into develop Apr 6, 2021
@akolotov akolotov deleted the audit/5.6 branch April 6, 2021 15:03
akolotov added a commit that referenced this pull request May 1, 2021
This update for the `master` branch contains the following set of changes:
  * [Improvement] Define token name suffixes at deploy time (#19), closes #18
  * [Fix] Use more robust message generation in fixMediatorBalance (#23), closes #22
  * [Fix] Minor refactoring and typos fixing (#24)
  * [Fix] Fix relayTokens for tokens supporting fees (#29)
  * [Fix] Add lock() checks to withdrawal handlers (#33)
  * [Fix] Use safeTransfer and safeMint wrappers (#34)
  * [Fix] Refactor readDecimals calls (#35)
  * [Fix] Use memory storage location modifier for variables that are processed in inline assembly (#38)
  * [Fix] Safe implementation of fees distribution (#40)
  * [Fix] Change call to staticcall for upgradeabilityOwner() (#42)
  * [Fix] Reduce gas cost in requestFailedMessageFix (#43)
  * [Other] Add publish step (#17), closes #16
  * [Other] Bump contracts interfaces version to 3.0.0 (#36), closes #28
  * [Other] Add a comment about required manual checks to setCustomTokenAddressPair (#37)
  * [Other] Comment about TUSD in fixMediatorBalance (#39)
  * [Other] Add a comment about if clause in _getMinterFor (#41)
  * [Other] Fix typos in comments (#45)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit team-april-2021 Related to items found by auditors which we called "Team April 2021"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants