neo-project / neo

NEO Smart Economy
MIT License
3.46k stars 1.03k forks source link

vm: Problem trying to PICKITEM from ByteString and then CAT ByteString and a single Byte picked from ByteString #2982

Open Hecate2 opened 11 months ago

Hecate2 commented 11 months ago

Contract:

        public static ByteString abiencode(ByteString data, bool reverse)
        {
            int len = data.Length;
            ExecutionEngine.Assert(len <= 32, "Too long data");
            ByteString prefix = "";
            for (int i = 0; i < 32 - len; ++i)
                prefix += "\x00";
            if (reverse)
            {
                for (int i = 1; i <= len; ++i)
                    prefix += data[len - i];
                return prefix;
            }else
                return prefix + data;
        }

Sample assembly:

# Code abi.cs line 19: "prefix += data[len - i];"
0109 LDLOC1
0110 LDARG0
0111 LDLOC0
0112 LDLOC2
0113 SUB
0114 DUP
0115 PUSHINT32 00-00-00-80 # -2147483648
0120 JMPGE 04 # pos: 124 (offset: 4)
0122 JMP 0A # pos: 132 (offset: 10)
0124 DUP
0125 PUSHINT32 FF-FF-FF-7F # 2147483647
0130 JMPLE 1E # pos: 160 (offset: 30)
0132 PUSHINT64 FF-FF-FF-FF-00-00-00-00 # 4294967295
0141 AND
0142 DUP
0143 PUSHINT32 FF-FF-FF-7F # 2147483647
0148 JMPLE 0C # pos: 160 (offset: 12)
0150 PUSHINT64 00-00-00-00-01-00-00-00 # 4294967296
0159 SUB
0160 PICKITEM
0161 CAT
0162 CONVERT 28 # ByteString type
0164 DUP
0165 STLOC1
0166 DROP

Invocation:

invokefunction('abiencode', [Hash160Str('0x06b9931e101f2e9885e76503874f2cebf69bf790'), True])
'{"jsonrpc":"2.0","method":"invokefunction","params":["0x65b9e0f9196acc8a58dad1a42fa459ecaf2b93c0","abiencode",[{"type":"Hash160","value":"0x06b9931e101f2e9885e76503874f2cebf69bf790"},{"type":"Boolean","value":true}],[{"account":"0x7651a826505c19ca20326381e96b0d5439519a0c","scopes":"CalledByEntry","allowedcontracts":[],"allowedgroups":[],"rules":[]}]],"id":1}'

Expected (64 hex chars, 32 bytes):

00000000000000000000000006b9931e101f2e9885e76503874f2cebf69bf790

Actual returned (many additional bytes 00):

00000000000000000000000006b90093001e101f2e98008500e700650387004f2ceb00f6009b00f7009000
                            !!  !!          !!  !!  !!      !!      !!  !!  !!  !!  !!

Explanation: At the instruction 0160 PICKITEM before 0161 CAT, NeoVM obtains an Integer from ByteString. This is guided by https://github.com/neo-project/neo-vm/blob/ea03316ae34ca4bc7a30fd4b65dad74f1fed7e17/src/Neo.VM/ExecutionEngine.cs#L1242-L1250 which casts a single byte to an Integer. Then in the following instruction CAT, we try to GetSpan() of the Integer, which is generated by https://github.com/neo-project/neo-vm/blob/ea03316ae34ca4bc7a30fd4b65dad74f1fed7e17/src/Neo.VM/Types/Integer.cs#L35 Here value.ToByteArray() may generate additional byte 00 when it's actually unnecessary. This is described by https://learn.microsoft.com/en-us/dotnet/api/system.numerics.biginteger.tobytearray?view=net-7.0#system-numerics-biginteger-tobytearray(system-boolean-system-boolean)

(And I cannot even come up with a workaround for the problem...)

Hecate2 commented 11 months ago

And neo-go does pick a single byte

https://github.com/nspcc-dev/neo-go/blob/3628b824e122b349a7411c657bb5ecda14340a48/pkg/vm/vm.go#L1255-L1264

dusmart commented 11 months ago

a solution to solve your problem: use public static byte[] Reverse(this Array source);

see https://github.com/neo-project/neo-devpack-dotnet/blob/a77456b47636acfebe1fc78cad57f6ebfdc60109/src/Neo.SmartContract.Framework/Helper.cs#L209-L215

Hecate2 commented 11 months ago

a solution to solve your problem: use public static byte[] Reverse(this Array source);

see https://github.com/neo-project/neo-devpack-dotnet/blob/a77456b47636acfebe1fc78cad57f6ebfdc60109/src/Neo.SmartContract.Framework/Helper.cs#L209-L215

Many thanks! The following contract works:

        public static ByteString abiencode(ByteString data, bool reverse)
        {
            int len = data.Length;
            ExecutionEngine.Assert(len <= 32, "Too long data");
            ByteString prefix = "";
            for (int i = 0; i < 32 - len; ++i)
                prefix += "\x00";
            if (reverse)
                return prefix + (ByteString)((byte[])data).Reverse();
            else
                return prefix + data;
        }
Hecate2 commented 11 months ago

And, also because of BigInteger adding an additional byte 0x00, (ByteString)BigInteger.Parse("63076024560530113402979550242307453568063438748328787417531900361828837441551") exceeds the 32-byte limit, if written in contracts. The purpose is to inject a constant hash 0x8b73c3c69bb8fe3d512ecc4cf759cc79239f7b179b0ffacaa9a75d522b39400f

AnnaShaleva commented 11 months ago

Just a question from my side:

Then in the following instruction CAT, we try to GetSpan() of the Integer, which is generated by https://github.com/neo-project/neo-vm/blob/ea03316ae34ca4bc7a30fd4b65dad74f1fed7e17/src/Neo.VM/Types/Integer.cs#L35 Here value.ToByteArray() may generate additional byte 00 when it's actually unnecessary.

Are you sure that exactly this part of code is responsible for extra zeroes? Because, as MSDN doc postulates, If the value is zero, returns an array of one byte whose element is 0x00.. And for the case of the zero integer, the Neo's conversion code provided above has value.IsZero ? ReadOnlyMemory<byte>.Empty check that will return an empty byte array if the source integer is zero.

Hecate2 commented 11 months ago

Zero integers are not related to the problem. The additional zeroes come from bytes of large values (probably those larger than 0x80?). If those large bytes are encoded with only 1 byte, they will be considered negative (minus) values, because the most significant bit is 1. So .NET encodes them with an additional byte 0x00 for them to be positive. For example, byte 0xb9 is encoded as 0xb900 (little-endian), because 0xb9 >= 0x80. (And then the little-endian bytes are inserted into a big-endian ByteString...)

shargon commented 10 months ago

We can change

Push((BigInteger)byteArray[index]); 

To create a ByteString instead of BigInteger