search

yieldprotocol / fyDai

Yield Protocol v1
GNU General Public License v3.0
44 stars 14 forks source link

Upgradeable Yield Proxy #377

Closed gakonst closed 4 years ago

gakonst commented 4 years ago

This PR separates the authorization flow of the proxy from the actual proxy implementation.

Users will always authorize 1 version of the proxy via the onboard and authorizePool calls, and then any subsequent calls will be forwarded to the address stored at IMPL_SLOT, which is set by an admin that's stored at the ADMIN_SLOT storage slots. These are made to be EIP1967 compatible to avoid storage collisions.

TODO:

gakonst commented 4 years ago

The YieldAuth contract is only responsible for batching permit / addDelegateBySignature calls together.

The YieldProxy contract inherits YieldAuth and implements the Unstructured Storage pattern. It forwards all calls to the actual implementation via delegatecall. This means that even though the contract has no explicit state variables (since it does not inherit ProxyStorage), the delegatecall makes it so that: YieldProxy calls ProxyV1, adjusting YieldProxy's storage, instead of ProxyV1's.

The ProxyStorage contract exists so that we can do "safe" upgrades by only appending storage slots. If you'd want to write ProxyV2 with new state variables, you'd create a new ProxyStorageV2 and you'd inherit from `. I do that in the mocks, and you can also see how Compound does this here.

The deployment process is:

  1. Deploy the implementation, ProxyV1.sol
  2. Deploy the proxy, YieldProxy.sol
  3. Call proxy.upgradeTo(implementation.address, <abi encoded initializer args>)
  4. Use proxy.address in the UI.

ProxyV1 is never exposed directly to the user. The user only permit/addDelegate's YieldProxy. Does that make things clearer?

alcueca commented 4 years ago

That makes things a lot clearer

alcueca commented 4 years ago

One thing that we need to implement from the very first PR is a timelock.

The first deployment will be fine, because users will have to authorize the upgradable proxy. From that time onwards, though, we could upgrade the contract in a way that allows us to steal all of our users' funds.

To avoid this, there should be at least a timelock on upgrades, and we should be scrupulous about how we communicate them.

alcueca commented 4 years ago

I think we are ready to merge this. I still have it grabbed by the fingertips, but I can develop new proxies and manage versioning and upgrades.

alcueca commented 4 years ago

Closed due to unsafe design.