Issue 2

[Edward-Lo]

Description

There is a lack of sanity check in the Upgrade() method of FlamingoSwapRouterContract which might lead to unwanted behaviour.

public static byte[] Upgrade(byte[] newScript, byte[] paramList, byte returnType, ContractPropertyState cps, string name, string version, string author, string email, string description)
{
    Assert(Runtime.CheckWitness(GetAdmin()), "upgrade: CheckWitness failed!");

    byte[] newContractHash = Hash160(newScript);

    Contract newContract = Contract.Migrate(newScript, paramList, returnType, cps, name, version, author, email, description);
    Runtime.Notify("upgrade", ExecutionEngine.ExecutingScriptHash, newContractHash);
    return newContractHash;
}

The Upgrade function calls Contract.Migrate() to upgrade the contract. Contract.Migrate() migrates everything in the persistent storage of the current contract to the new contract when executed. For Migrate() method, it will only transfer the contract storages when the target contract has not been deployed yet.

Specifically, one can frontrun the deployment of the new contract so the migration won’t transfer the storages to the new contract. Though what an attacker can do still depends on the new contract, this might not be the operator’s expectation.

Recommendation

Check whether the contract already exists before calling Contract.Migrate().

[Ashuaidehao]

Updated,thanks for your suggestion.