Issue 3

[Edward-Lo]

Description

There is a lack of sanity check in the Upgrade() method of FlamingoSwapPairContract 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!");

    var me = ExecutionEngine.ExecutingScriptHash;
    byte[] newContractHash = Hash160(newScript);

    var r = ReservePair;
    SafeTransfer(Token0, me, newContractHash, r.Reserve0);
    SafeTransfer(Token1, me, newContractHash, r.Reserve1);

    Contract newContract = Contract.Migrate(newScript, paramList, returnType, cps, name, version, author, email, description);

    Runtime.Notify("upgrade", ExecutionEngine.ExecutingScriptHash, newContractHash);
    return newContractHash;
}

The Upgrade function first transfers the tokens to the new contract, then 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(). And transfer the tokens after the contract migration has succeeded.

[Ashuaidehao]

Updated,thanks for your suggestion.