MKR transferred directly to the proxy can only be withdrawn back to the "cold" wallet, not locked straight into the voting contract

[MicahZoltu]

If you transfer MKR directly to the VoteProxy, it will get "stuck" and be unusable for voting until you call proxy.freeAll, at which point it'll be refunded to the cold wallet.

This could be fixed by changing: https://github.com/makerdao/vote-proxy/blob/ebd7b2f484bc18d42563b443fbad05702d0abb38/src/VoteProxy.sol#L30-L33

to:

function lock(uint256 wad) public auth {
    gov.pull(cold, wad);
    chief.lock(gov.balanceOf(this));
}

Such a change will make it so the next time someone calls lock(0), it will include any MKR held by the proxy contract directly.

Note: It should be verified that gov.pull(cold, 0); will not error. If it does, a different solution would be called for.

[nmushegian]

It should be verified that gov.pull(cold, 0); will not error. If it does, a different solution would be called for.

Hmm, I think it does.

Seems like using balanceOf(this) and pushing to the owner on free is the right approach after all

[nmushegian]

Another approach is a function like clear or top-up which does only chief.lock(gov.balanceOf(this));

[MicahZoltu]

For reasons outside of the specific issue of "accidentally inaccessible funds", I would like to see a solution that makes sending MKR directly to VoteProxy a viable strategy.

Imagine you use paper wallets for cold storage. Currently, you cannot use VoteProxy because you cannot call gov.approve(vote_proxy, ...) from the paper wallet without burning it, at which point your funds are no longer secure. If there was a function that allowed you to chief.lock(gov.balanceOf(this)) then you could do something like:

1. Create new_paper_wallet.
2. Create vote_proxy pointing at hot_wallet and new_paper_wallet.
3. Transfer MKR from old_paper_wallet to vote_proxy (burning old_paper_wallet in the process).
4. Use hot_wallet to trigger locking of the MKR that is in vote_proxy.

You would end up with an un-burned paper wallet (new_paper_wallet) being the indirect owner of the assets.

[nmushegian]

Latest consensus is

lock uses gov.balanceOf(this)

add rake:

    function rake(uint256 wad) public auth {
        chief.lock(wad);       // mkr out, ious in
    }

Will put a PR in the morning.

[MicahZoltu]

What is the reasoning behind rake taking a parameter? Why not have it just lock everything in the contract? What is the scenario where you would not want rake to lock everything?

[nmushegian]

What is the scenario where you would not want rake to lock everything?

Same discussion as explicit lock/free in https://github.com/makerdao/vote-proxy/pull/8

Hopefully that PR supersedes this whole issue