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

Fix security issue of extractStuckERC20 #1

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

gorbunovperm
Copy link

No description provided.

@gorbunovperm
Copy link
Author

This is a fix based on a vulnerability found by @u59149403 :

Okay, now I found actual bug. Let's assume that 0x01000B5fE61411C466b70631d7fF070187179Bbf address owner (presumably this is you) will call TokenStandardConverter.extractStuckERC20 with original ERC-223 token (for which ERC-20 wrapper exists) as an argument. Then you will be able to withdraw all amount of this token.

It is described here ethereum#658 (comment)

@gorbunovperm
Copy link
Author

The following is a description of the fix logic.

The converter contract can be deposited with tokens such as:

  1. ERC-20. Purposely, through wrapERC20toERC223 function, the incoming number of tokens will be taken into account: erc20Supply[_ERC20token] += _amount.

  2. ERC-20. Accidentally, then it will be possible to save tokens through extractStuckERC20. Even if there were purposely deposits before, thanks to erc20Supply[_ERC20token], only accidentally deposited tokens will be extracted.

  3. ERC-223 original. Purposely or accidentally, in any case, the tokens will be processed by tokenReceived. An erc20 wrapper will be created there and the variable erc20Wrappers[_token] will have a wrapper address. The non-zero value of this variable is the exact marker that _token is erc223. We prohibit extracting such tokens in extractStuckERC20. (the essence of this fix)

  4. ERC-223 wrapper. Purposely or accidentally. These tokens are burned in tokenReceived when they arrive. We don't need to worry about their withdrawal via extractStuckERC20.

@Dexaran Dexaran merged commit 5b61fa1 into Dexaran:master Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants