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

Ability to rescue tokens and ETH sent to contract address #94

Closed
marsrobertson opened this issue Mar 3, 2021 · 10 comments
Closed

Ability to rescue tokens and ETH sent to contract address #94

marsrobertson opened this issue Mar 3, 2021 · 10 comments

Comments

@marsrobertson
Copy link

marsrobertson commented Mar 3, 2021

USDCv2 has option to recover funds

USDC proxy: https://etherscan.io/token/0xa0b86991c6218b36c1d19d4a2e9eb0ce3606eb48#readProxyContract

Implementation: https://etherscan.io/address/0xb7277a6e95992041568d9391d09d0122023778a2#code

Starts at line 1313:

contract Rescuable is Ownable {
    using SafeERC20 for IERC20;

    address private _rescuer;

    event RescuerChanged(address indexed newRescuer);

    /**
     * @notice Returns current rescuer
     * @return Rescuer's address
     */
    function rescuer() external view returns (address) {
        return _rescuer;
    }

    /**
     * @notice Revert if called by any account other than the rescuer.
     */
    modifier onlyRescuer() {
        require(msg.sender == _rescuer, "Rescuable: caller is not the rescuer");
        _;
    }

    /**
     * @notice Rescue ERC20 tokens locked up in this contract.
     * @param tokenContract ERC20 token contract address
     * @param to        Recipient address
     * @param amount    Amount to withdraw
     */
    function rescueERC20(
        IERC20 tokenContract,
        address to,
        uint256 amount
    ) external onlyRescuer {
        tokenContract.safeTransfer(to, amount);
    }

    /**
     * @notice Assign the rescuer role to a given address.
     * @param newRescuer New rescuer's address
     */
    function updateRescuer(address newRescuer) external onlyOwner {
        require(
            newRescuer != address(0),
            "Rescuable: new rescuer is the zero address"
        );
        _rescuer = newRescuer;
        emit RescuerChanged(newRescuer);
    }
}
@Ro5s
Copy link
Contributor

Ro5s commented Mar 3, 2021

thanks for flagging @marsrobertson and including reference implementation 🙂. I guess I would wonder, "who would be the owner of WETH10 contract?"

Token 'rescue' programs like those in USDC contemplate an admin owner of the contract (Ownable) that detects and decides on token recoveries. While this is a great feature for users, esp. in the context of "centralized" stablecoins where control is always assumed in account backing tokens with dollars, it can complicate the situation for many other tokens.

@marsrobertson
Copy link
Author

I guess I would wonder, "who would be the owner of WETH10 contract?"

Is there a possibility of doing this without the admin?

For ERC20 I don't think so, but maybe there is a way?

I'm embodying a mindset of finding a solution, rather than focusing on why something cannot be done.

image

A workaround would be to use 4-of-7 multisig of maintainers with $1000 recovery fee.

@wighawag
Copy link
Collaborator

wighawag commented Mar 8, 2021

Actually, we could add a mechanism where you provide the proof of the existence of the ERC20 Transfer events that caused the ERC20 to end up in the WETH contract

To verify this the WETH contract will need to be able to get the hash of the block where the event took place, this means your retrieval transaction need to happen in less than 256 block after the erroneous transfer. We could also allow user to simply record a previous block hash, to give time to later provide the proof. But assuming we have tooling, this recording is not necessary.

Wallets could have the tooling embedded so if you realize quick enough you send to the wrong destination you can submit the retrieval tx

But really Wallets should prevent you from sending the token to WETH in the first place :)

So in practise this might NOT be a good solution

@marsrobertson
Copy link
Author

But really Wallets should prevent you from sending the token to WETH in the first place :)

That tooling can be a standard EIP.

As simple as: IF sending ERC20 to ERC20 contract address THEN error

Is there a single example in the history of Ethereum where you actually want to send tokens to the token contract address?

@alcueca
Copy link
Contributor

alcueca commented Mar 11, 2021

In the deployed implementation, if you send WETH10 tokens to the WETH10 contract it becomes a withdrawal.

  1. Send ETH to WETH10 -> Wrap to WETH10
  2. Send WETH10 to WETH10 -> Unwrap to ETH

I took this functionality from USMFUM. It means that you can wrap and unwrap between Ether and WETH10 using only metamask transfers.

Preventing users sending other tokens to WETH10 I think will cost more (in complexity or gas) than is worth.

@bitdeep
Copy link

bitdeep commented Jun 15, 2021

WETH10 implements IWETH10 that's implement IERC20, so it's a ERC20 contract.

What if admin pass same WETH10 contract as tokenContract to rescueERC20? Then admin can transfer WETH10 any arbitrary address?

One better solution:

    function rescueERC20(
        IERC20 tokenContract,
        address to,
        uint256 amount
    ) external onlyRescuer {
        require( tokenContract != address(this), "Can't transfer WETH10." );
        tokenContract.safeTransfer(to, amount);
    }

@alcueca
Copy link
Contributor

alcueca commented Jun 20, 2021

Eth sent to the contract address becomes WETH, WETH sent to the contract address becomes Eth.

Only we messed up slightly in WETH10 :P

@alcueca alcueca closed this as completed Jun 20, 2021
@cyanlink
Copy link

cyanlink commented Jan 31, 2022

...
But really Wallets should prevent you from sending the token to WETH in the first place :)

So in practise this might NOT be a good solution

@wighawag
No they did not 😢, it was metamask the victim was using resulting in a 500k USD loss in WETH.
See my reddit post.
See my proposal at Metamask community

@marsrobertson
Copy link
Author

https://www.reddit.com/r/ethereum/comments/sfz4kw/did_i_just_lose_half_a_million_dollars_by_sending/
image

https://news.ycombinator.com/item?id=30134500
image

It causes sentiment like this

https://news.ycombinator.com/item?id=30120437
image

My reply:

Blockchain is the most impactful technology since the internet.
It changes everything - foundation of trust. New economy. New incentives.
“If you don't believe it or don't get it, I don't have the time to try to convince you, sorry.”
I'm pretty sure some people were angry at the internet 30 years ago.

That's why UI / UX etc...

@cyanlink
Copy link

cyanlink commented Feb 2, 2022

@marsrobertson MetaMask/metamask-extension#12642 bug discovered last year, fix development near completion, PR not passed & merged = huge user loss. Not blaming the metamask team tho, they were doing software engineering things, QAs and code reviews, it took time.
In this incident who is responsible: metamask (bug discovered, but not fixed asap) - binance academy (misleading introduction to newbies) - weth.io (using metaphors to describe, no direct user guidance) - original contract (least fault, no fool-proof design) - user (least fault, newbie who does not fully understand smart contracts)

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

No branches or pull requests

6 participants