Soroban doesn't have a good story for updating the contract implementation yet and we'd like to provide better support before v1 release (otherwise the v1 contracts would be stuck with their implementation forever).
I wrote up a small doc listing the possible solutions: https://docs.google.com/document/d/1pCBhl1B7FMgzxUnssS2wws77jw_H89jgekyNLrEmdP8/edit# . tl;dr is that we either do storage delegation support in order to just support contract-based proxies, or allow updating the contract implementation direclty (I'm in favor of this and probably tentatively would vote for solution 2 from the doc with leaving 3 for the future extension). I'm curious to see what the others think and if there are any other approaches I might've missed
#Updating contract implementation
dreamy frost
cc @sage escarp @lament fox @fallow cove @safe lark
fallow cove
I think I'm partial to option 3 since it does allow bulk updates
(and I think we're assuming a model with quite a lot of "instances" of the same "class")
dreamy frost
that's fair - I just feel that option 2 is probably a bare minimum we need for the release. but if the rent doesn't get too complicated in 3, it would be better
fallow cove
I also think that there's a bit of a risk to users in code changing underfoot, and they should be able to say "call this contract if its hash is still $X otherwise fail"
i.e. some users will want to "lock versions" and pay the risk of being out of date
(I guess the contract itself could do a version-lock by just not marking itself as delegating)
but I think tx invokers / auth signatures might want an option to explicitly lock version too
dreamy frost
I don't think we should allow users to pick version (imagine vX having vulnerability that has been fixed by vY - I wouldn't want anyone to use vX anyomore). we could indeed add wasm hash to the signature payload, but I'm not sure if that provides enough meaningful control (I guess that depends on existence of some DBs with 'trusted' Wasms)
but I don't think it would cause much harm either (besides some additional computation/size impact)
sage escarp
I think we're assuming a model with quite a lot of "instances" of the same "class", but bulk updates with Solution 3 only help if there are many "instances" of the same "class" controlled by the same entity that can update the single proxy instance. Do we have an idea of how common this model is? The AMM example with a single admin across pools is a good one, but I'm curious if we're optimizing for a minor use case. For example, users can deploy our token example, but we expect the admin to usually be different between the tokens.
Without solution 3 bulk updates could still be accomplished using multiple invoke host function operations or a custom "bulk update" contract right? I'm not sure if Solution 3 is worth the complexity over Solution 2.
dreamy frost
the complexity is largely contingent on the rent, otherwise there isn't much difference. as for the use cases, I'm not entirely sure. there is some merit to using the implementation contract 'as a library' and get the upgrades automatically (that's not much different from the SAC users being updated at once if SAC implementation changes), but that obviously comes with security tradeoffs.
sage escarp
Well it's not just implementation complexity, but also the complexity of expanding the set of contract executable types the user needs to be aware of. It's not a big deal, but why give the user more to think about when we don't need to. This is assuming bulk updates aren't common and can be done in other ways like I mentioned.
dreamy frost
that's fair, there is definitely some additional UX complexity. that said, proxies/upgradeability is a pretty advanced feature already, so I'm not too worried necessarily (and the upside is that we have a better story for someone who e.g. wants to build a complex dex on soroban). I'm not feeling strongly either way; as I've mentioned I feel like just allowing upgrading wasm is good enough to start with - it makes migration to more advanced patterns possible
hoary quiver
I agree with @fallow cove that there are risks associated with updating contracts. We need to have a good story to mitigate a scenario like:
- Deploy fork of uniswapV2
- Wait for users to deposit funds
- Update contract, steal fund
We could think of a design where a new executable is submitted but is not active for a certain period of time. This allows the users to audit the new code and decide to remove their funds or not (in the AMM example). What do you think?
About your point:
Even without considering the case for introducing the new features, there needs to be the possibility of patching vulnerabilities on-chain
If there is a vulnerability in a contract, we could also imagine that it is the responsibility of the Dapp to redirect the users to a new contract and provide the right UX to transition the data from the old to the new contract.
In my opinion, this feature can open a can of worms and it significantly expand the attack surface so we need to be sure that this is a must have feature for Soroban.
haughty plaza
I agree with <@938163908852732007> that there are risks associated with updating...
I think (if I understand solution 2 correctly) that this is a problem that shouldn't have an impact in the decision for implementing solution 2 or not. After all, the contract would be opening up an update function. Usually, I'd expect a function that performs such action to be authorized only under certain conditions, i.e in a DeFi protocol the contract can be upgraded only when governance says so. If a user deposits money in a contract that has an upgrade function that can be triggered by a central authority that's up to them, just like putting money in a contract that has a transfer function that can transfer all the funds.
dreamy frost
We need to have a good story to mitigate a scenario like:
- Deploy fork of uniswapV2
- Wait for users to deposit funds
- Update contract, steal fund
We could think of a design where a new executable is submitted but is not active for a certain period of time.
this won't help in the described scenario as the only thing the malicious party needs to do is to update the code to let them withdraw funds themselves. but OTOH it's not that different from just deploying a contract with a 'backdoor' that allows withdrawing funds from it.
. This allows the users to audit the new code and decide to remove their funds or not (in the AMM example).
that won't work. don't forget that the balances are managed by the token contracts (not to mention that we can't distinguish between the malicous amm admins and amm users).
If there is a vulnerability in a contract, we could also imagine that it is the responsibility of the Dapp to redirect the users to a new contract and provide the right UX to transition the data from the old to the new contract.
that's asking too much from the users and doesn't fit well into the contract user story and the ledger usage model. also besides data that is owned by the contract, there is also data that belongs to that contract, but owned by another contract (notably, this includes token balances).
FWIW from the feedback I've got so far, I've got an impression that pretty much every important contract uses proxies for the sake of upgradeability.
In my opinion, this feature can open a can of worms and it significantly expand the attack surface so we need to be sure that this is a must have feature for Soroban.
I agree that there is non-zero attack surface increase. but instead we should consider the usability vs security tradeoff - if the system is somewhat more secure, but unusable for a lot of use cases (and specifically the use cases that will be needed for complex and useful contracts like uniswap), then I'd say that probably it's worth to go for the tradeoff
there is a few of things that help mitigating the risks:
- it is easy to audit the contracts for upgradeability (I don't think it's possible to obfuscate this - the contract has to use the specific host function and it's possible to identify such import/call in wasm
- it is possible to include wasm hash into signature to at least flag the contract upgrades
- option 3 can actually help limiting trust to a single party. e.g. if instead of distributing uniswap contract via wasm, there was a single party that controls the 'source' contract, then the users only need to trust that party
maybe there are more things we can do - I think we should rather think about them instead of just discarding the feature and relying on the users coming up with hacky and even less secure systems
brisk stag
Just curious, would the SAC (and any standard, out of the box contracts provided by the protocol) be upgradeable and would they follow the same upgrade mechanism as one of these options?
dreamy frost
SAC upgrade would be a protocol upgrade and handled in the same fashion as any other protocol upgrade (i.e. it would be conditioned on the protocol version change, which happens via validator vote)
(unless the update is no-op, that is)
so e.g. if we wanted to add a function to the SAC interface, we'd need to wait for the next protocol release. the same is true for any host fn change, btw
hoary quiver
this won't help in the described scenario as the only thing the malicious party needs to do is to update the code to let them withdraw funds themselves
I may be missing something but I don't understand why that won't help.
To be clear I was thinking of something like this:
- ContractA-v0 exists on chain. Users can audit the code, verify that there is no backdoor and decide to use it
- Owner wants to upgrade to it to ContractA-v1. The code is deployed on chain but not active. I.e users still interact with ContractA-v0.
- Users can audit the code of ContractA-v1 that exists on chain and verify that there is no backdoor
- After X ledgers, the host (the owner cannot trigger this operation) makes ContractA-v1 active. Only then ContractA-v0 is updated with ContractA-v1.
The main goal here is to offer some time to the users to opt out if the new contract is not acceptable to them.
This solution is used by StarkWare for example: https://medium.com/starkware/contract-upgradability-d3a4451877c
it's not that different from just deploying a contract with a 'backdoor' that allows withdrawing funds from it.
The difference is that people can audit the code before using the contract. If a contract is updated an introduce a backdoor, it is not possible for the users who are already using the contract to do anything.
dreamy frost
The difference is that people can audit the code before using the contract.
but if the contract doesn't allow withdrawing funds, then this would do nothing
the article describes a higher level system
and I think it can be achieved via option 3 from my doc
but I don't think it's feasible at the protocol level
as we can really put any requirements on the contract interface and the user funds; not to mention that 'funds' are not necessarily represented by the tokens
hoary quiver
If there is a vulnerability in a contract, we could also imagine that it is the responsibility of the Dapp to redirect the users to a new contract and provide the right UX to transition the data from the old to the new contract.
that's asking too much from the users and doesn't fit well into the contract user story and the ledger usage model.
Fair enough I agree.
*. This allows the users to audit the new code and decide to remove their funds or not (in the AMM example). *
that won't work. don't forget that the balances are managed by the token contracts
I think I am missing something here. What's preventing the users from removing their funds if they realize that the new contract is not acceptable?
dreamy frost
there is no default way of withdrawing the funds from the contract and there can't be one at the protocol level (as there is no concept of 'funds' as well)
hoary quiver
Yes sure. I was assuming the original contract had a withdraw function
dreamy frost
I'm not necessarily against the time-locked updates; I realize that they might be useful for a significant chunk of the contracts. that said, I'm not sure this logic should belong to L1 - it would be a pain to implement on-chain (as we don't really support operation scheduling) and it also doesn't really solve all the issues holistically
which is why I'm coming back to option 3 that allows to do exactly what is suggested in that article in a customizable way and with arbitrarily defined rules for the update
(like you can introduce voting for a source update, or do timelocks, or enforce withdrawal etc.)
FWIW option 2 is valid as well, but it has more narrow use case, as it doesn't allow fully implementing the proxy pattern
hoary quiver
Yes that makes sense. The only difference is that this is not enforced by the protocol so contract owners may choose to do it or not. But as long as the have the ability to do it, that's a good design in my opinion.
dreamy frost
yeah, and it shouldn't be hard to flag upgradeable contracts when using the wallet. contract safety is a problem that we generally can't solve at the protocol level, so I guess the best we can do is provide the necessary tools
native rune
This conversation (which is great) reminds me of a quote from a podcast on smart contract design i heard recently:
“Code is law but interfaces are a crime”. I interpret this as meaning that its difficult to enforce things even via interfaces since people will fork and custom impl stuff anyway.
dreamy frost
https://github.com/stellar/rs-soroban-env/pull/745 implements option 2 - it's really simple and it seems like we should be ok with having this
lament fox
yeah this looks workable. Another benefit of option 2 (I think), is that it supports upgrades with data migration (as it's only upgrading one instance at a time, that will be bounded). So you can submit a transaction that does: {op_1: my_instance.upgradeWASM(), op_2: my_instance.schemaMigration()} (I think that's the way to call the migration code that is in the new WASM)
hoary quiver
allows to do exactly what is suggested in that article in a customizable way and with arbitrarily defined rules for the update
How would a dev implement the time locked update in option 2 or 3? I am not sure I see how it is possible to put a code on chain that will not be used and be here only for users to audit.
dreamy frost
yeah, you can also migrate schema if needed, though I suppose that would typically need to happen in a lazy fashion (unless you have absolute control over all the entries, which isn't true in a general case)
I think extending this to also support 3 is easy enough - I can give it a shot as well - that would better support the governed upgrades, like the protocol described above.
How would a dev implement the time locked update in option 2 or 3?
FWIW option 2 is a subset of 3 and will be supported anyway. the time-locked updates are supported as well, if needed. e.g. the contract exposesschedule_upgradefunction that creates a ledger entry with the time bound after which the upgrade is valid. then it exposesexecute_upgradethat requires the scheduled upgrade to be present on-chain for the defined time. scheduling call ofexecute_upgradehas to happen off-chain
(which is our general strategy for scheduling operations)
hoary quiver
Just looked at your PR:
Wasm entry corresponding to the hash has to already be present in the ledger
I thought the WASM was part of the same host function. That sounds good thanks for the additional details.
dreamy frost
yeah, contracts don't store their wasm
hoary quiver
What are the implications for the auth framework? Does an authorization still hold when a contract is updated?
lament fox
In the original auth sketch, the WASM hash was part of the auth context, with this change we should make sure it's the case.
dreamy frost
we can add wasm hash to the signature payload. I'm still a bit partial on that though, as it's not clear to me how actionable this is. but I'm not opposed adding it either, though at this point we may want to start hashing the auth payload (as that's another +32 bytes/contract call per transaction)
hoary quiver
Yes I'd recommend adding it. For use case like subscriptions discussed in the "Extensible accounts" thread, that would mean users would need to reauthorize the delegation. From a security perspective I like it but I am curious how people feel from a UX point of view.
dreamy frost
actually the delegation support would be separate; for now I'm just adding the executable to the signature payload
(but I'm ok with supporting that for delegation as well, if we support it at all, of course)
lament fox
it's going to cause all transactions following the upgrade (both in the same ledger and in flight) to fail by default which sounds like the right thing to do. I can see having some implementations of check_auth that would "opt out" of this for some trusted instances, but I am not sure how much of a pain it would be to allow to do this.