neo: `ushort` `ContractState.UpdateCounter` overflow

using a new contract id.

This is transparent to the user, the address could be the same. I think that overflow UpdateCounter is not problematic, is just a counter, updating the contract the owner could produce more damage to the user.

what you mean destroy + recreate? are you saying redeploy? wont operation like recreate with new contract code generates another contract address For me,different contract address means different contracts~~~

Yes, I meant destroy + recreate.

The contract address is derived from NEF checksum + contract manifest name + account that deployed the contract. So it is possible to destroy a contract and then redeploy it and get the same contract address.

Here’s a set of steps that could be used to trick people into calling a malicious contract

1. Deploy non-malicious version of the contract
2. Update contract one or more times to increment UpdateCounter. These updates must also be non-malicious
3. Promote contract usage, gain adoption
4. Destroy contract
5. Re-deploy non-malicious version of contract from step 1
6. update contract same number of times from step 2, but this time to a malicious version

You now have a malicious contract deployed in a manner that cannot be detected.

Is there a way to ensure a destroyed contract address is not reused? I think disabling contract address reuse combined with explicitly checking for overflow when incrementing UpdateCounter would mitigate this issue

by keeping updating the smart contract until itsUpdateCounter jumps back to 0

You can destroy and create the contract again to get UpdateCounter=0

the script hash of the overflowed contract is: 0x9f89a0f5de46a085c99e6ef54a977bb37b2099b1 on neo3 testnet

It’s always better to test this things in private net

i think there would be no critical risk, however, it can cheat users or explorers.

i.e. onegate browser has a verified source code section and can be cheated through this way: https://testnet.explorer.onegate.space/contractinfo/0x9f89a0f5de46a085c99e6ef54a977bb37b2099b1 (however it is crashed before the cheat kkkk)

to fix it, the cheapest way can be: change the following line

https://github.com/neo-project/neo/blob/1e449185c0d05e9824c257ab4404f380106d15b2/src/neo/SmartContract/Native/ContractManagement.cs#L239

to

checked { contract.UpdateCounter++; }

Being able to reset the UpdateCounter to zero, either by overflow or destroy + recreate, seems to defeat the purpose of having UpdateCounter. I get asked regularly (including in my devlog presentation today) about the security risk of supporting updatable contracts. My usual response is that the update counter can be checked to ensure the contract is the expected version. But if that value can be UpdateCounter manipulated, an attacker could disguise their contract update and trick users into invoking a malicious contract.

Maybe we should be exposing the NEF checksum or a SHA of the NEF script contents that could be used for optionally validating script contents for developers that wish to guard against this kind of attack?

cc @erikzhang @Liaojinghui