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

Unable to set token wallet to reserve contract #415

Open
Anyhowclick opened this issue Jul 29, 2019 · 2 comments
Open

Unable to set token wallet to reserve contract #415

Anyhowclick opened this issue Jul 29, 2019 · 2 comments

Comments

@Anyhowclick
Copy link
Contributor

Once token wallet is set, there is no way to set it to the reserve contract anymore. This is because token approval only happens in approveWithdrawAddress, and has the condition that the token wallet address is null, but setTokenWallet checks that the input address is not null. As such, there is no way to reset the token wallet.

Code reference:

function approveWithdrawAddress(ERC20 token, address addr, bool approve) public onlyAdmin {
        approvedWithdrawAddresses[keccak256(token, addr)] = approve;
        WithdrawAddressApproved(token, addr, approve);

        setDecimals(token);
        if ((tokenWallet[token] == address(0x0)) && (token != ETH_TOKEN_ADDRESS)) {
            tokenWallet[token] = this; // by default
            require(token.approve(this, 2 ** 255));
        }
    }

    function setTokenWallet(ERC20 token, address wallet) public onlyAdmin {
        require(wallet != address(0x0));
        tokenWallet[token] = wallet;
        NewTokenWallet(token, wallet);
    }

Recommendation is to have a conditional check for token approval in setTokenWallet:

    function setTokenWallet(ERC20 token, address wallet) public onlyAdmin {
        require(wallet != address(0x0));
        tokenWallet[token] = wallet;
        if(wallet == address(this)) {
           require(token.approve(this, 2 ** 255));
        }
        NewTokenWallet(token, wallet);
    }
@yaronvel
Copy link
Contributor

Should probably reset old wallet allowance in this case. And also reset allowance to reserve in case it was non zero before. More generally if u want to change it, then probably should support proper change of token wallet. And not only for wallet as reserve.

@Anyhowclick
Copy link
Contributor Author

Oh right....

In that case, I suppose a separate function to change token wallet would be best?

function changeTokenWallet(ERC20 token, address wallet) public onlyAdmin {
        require(wallet != address(0x0));
        if (wallet == address(this)) {
           require(token.approve(this, 0)); //reset allowance first
           require(token.approve(this, 2 ** 255));
        } else {
           //check that old wallet allowance has been reset
           require(token.allowance(tokenWallet[token], this) == 0);
        }
        tokenWallet[token] = wallet;
        NewTokenWallet(token, wallet);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants