Map does not respect K,V types

[ixje]

Background
I'm helping the GhostMarket team debug why a transfer event is emitted with a ByteString for amount instead of an Integer (as should be per NEP-17 spec) to prevent problems once the Notify ABI validation PR gets merged.

We have 3 transactions on T5 that can be summarized as

1. Contract A calls transfer on Python Nep-17 contract, emits amount as ByteString
2. Contract A calls transfer on non-native C# Nep-17 contract, emits amount as ByteString
3. Contract A calls transfer on native Nep-17 contract, emits amount as int

ContractA is a C# contract where the ClaimTradingFees function is called

public static bool ClaimTradingFees (UInt160 currency) 
    {
        var balances = GetFundBalancePerCurrency();
        BigInteger amount = balances[currency];
        Expect(Nep17Transfer(currency, Runtime.ExecutingScriptHash, GetGfundHash(),
                                amount), "ClaimTradingFees NEP-17 transfer failed");

        return true;
    }

public static Map<UInt160, BigInteger> GetFundBalancePerCurrency()
    {
        Map<UInt160, BigInteger> map = new();

        StorageMap _fundBalanceContractMap = new(Storage.CurrentReadOnlyContext, FUND_BALANCE_PREFIX);
        var iterator = _fundBalanceContractMap.Find(FindOptions.RemovePrefix);

        while (iterator.Next())
        {
            var kvp = (object[])iterator.Value;
            var key = (UInt160)kvp[0];
            map[key] = (BigInteger)kvp[1];
        }

        return map;
    }

 private static bool Nep17Transfer(UInt160 baseScriptHash, UInt160 from, UInt160 to, BigInteger amount, object data = null)
    {
        return (bool)Contract.Call(baseScriptHash, "transfer", CallFlags.All, from, to, amount, data);
    }

Issue

I found that the cast to BigInteger has no effect once translated to nvm code.

            var kvp = (object[])iterator.Value;
            var key = (UInt160)kvp[0];
            map[key] = (BigInteger)kvp[1];

In other words; it it retrieved from the iterator.value as a ByteString and it is stored as a ByteString in the map. When executed it looks like this

SYSCALL System.Iterator.Value \
RET                           |
STLOC3                        / stloc3 = "var kvp = iterator.value"
LDLOC3
PUSH0    \ kvp[0]
PICKITEM /
STLOC4   # store key
LDLOC3   # load kvp
PUSH1    \ kvp[1] (=amount)
PICKITEM / 
DUP      # amount copy
LDLOC4   # key
LDLOC0   # map
REVERSE3
CALL_L
SETITEM  # map[key] = amount (still byte string)
RET
DROP
NOP
JMP_L
NOP
LDLOC2  # iterator
CALL_L
SYSCALL System.Iterator.Next

No where in the execution do we see a CONVERT to IntegerStackItem to respect the BigInteger value type of the map.

The same holds for reading the value from the map here

BigInteger amount = balances[currency];

Which looks just like this

STLOC0
LDLOC0  # balances
LDARG0  # currency
CALL_L
PICKITEM  # BigInteger amount = balances[currency];

and no CONVERT happens after it anywhere.

The reason transaction 3. (to a native contract) emits amount as integer is because it does parameter conversion here prior to calling transfer()

solution
One option is for contracts to perform parameter type checking. However, I still think that the Map K,V types could and should be respected otherwise it becomes really hard to reason about the code you're writing.

[ixje]

I think this could have also been prevented by neo-project/neo#1891 (which is closed for what seems like no apparent reason)

[ixje]

This issue actually hits a similar problem where it cannot trust that what is returned from storage is converted back to the type one wishes

[superboyiii]

var kvp = (object[])iterator.Value;
            var key = (UInt160)kvp[0];
            map[key] = (BigInteger)kvp[1];

This convert to Integer from obj
It will not work in vm, need convert to ByteArray first then Integer

[ixje]

var kvp = (object[])iterator.Value;
            var key = (UInt160)kvp[0];
            map[key] = (BigInteger)kvp[1];

This convert to Integer from obj It will not work in vm, need convert to ByteArray first then Integer

I don't understand your reply. The above code is a snippet from a contract that is deployed by GhostMarket. That means it compiled without errors and therefore giving the programmer the false idea that the casting works (because the analysis in my post shows it actually doesn't do anything).

[vncoelho]

Let's move this forward, @ixje.

@superboyiii,
Thus, current something like map[key] = (BigInteger)(ByteArray)kvp[1]; would work, right?
Do you think we should add a new procedure for devpack to jump this extra conversion and do that automatically?

[vncoelho]

Perhaps it has been closed by #835 (comment)

[Jim8y]

Close as completed

[ixje]

I don't believe this is fixed by #835. #835 is a work around but everything said in #769 (comment) is still valid. Unless some warning is printed when using code like in the snippet in the comment people will continue to face this problem.

[Jim8y]

I will double check, you can use getinteger now.