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

Remove ownership changing code in favor of an owned mailbox. #25

Closed
MicahZoltu opened this issue Dec 27, 2019 · 0 comments
Closed

Remove ownership changing code in favor of an owned mailbox. #25

MicahZoltu opened this issue Dec 27, 2019 · 0 comments

Comments

@MicahZoltu
Copy link

MicahZoltu commented Dec 27, 2019

https://github.com/Uniswap/uniswap-v2-core/blob/d2ddbd95e8500cfd3669ab81b64f48956521ea6b/contracts/UniswapV2Factory.sol#L56-L59

Problem

Since the owner can change at any time, every time fees need to be paid the transaction must first lookup the fee recipient address on the factory. This results in both a storage lookup and a remote contract call which is somewhat gas expensive.

Solution

Before deploying Uniswap Factory/Exchange, first deploy an owned mailbox. Hardcode the address of that mailbox into Uniswap Factory, which would then embed it into Uniswap Exchanges when they are created. This will remove the need to do the lookup which removes both the remote call and the SLOAD. The mailbox itself can have its owner changed at any time, but fee payers can ignore this.

The mailbox can be a simple owned contract that allows delegated execution. There are a number of such already-audited and widely-used contracts that you could use with the simplest being an Owned contract with an execute function on it (note: the recoverable-wallet code has been audited by OpenZeppelin, but you could crib the same code from a number of different projects like Gnosis Safe or various multisig wallets). One caveat is that for some token types (I think 1155 does this) you may need to expose a particular method on the contract to mark it as being a valid receiver of that type of token. In the case of Uniswap fees, you presumably just need to match any such checks Uniswap has since fees will always be in a Uniswap supported token.

Another option for the mailbox could be one that simply allows ERC20, ERC777, ERC1155, and ETH withdraws by the owner. This is even simpler, though it does mean if you receive some weird token as a fee with bizarre requirements for withdraw they may get stuck in a way that an arbitrary executor could recover from.

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

2 participants