Dust in attachedDeposit

[lunap2p]

with attachedDeposit
exam: 5 Near
When I make a function call that gets the amount I attached, I always get a number greater than 5

Why does some dust appear?
2594073385365405696
but if i attach 10 near, this dust number does not appear

I think this is a strange problem
It happens with 5 but not with 10
I tried some other numbers and some numbers come up and some don't

[ailisp]

Hi @lunap2p! Very interesting finding! I'm able to reproduce your result with exact number: 5000002594073385365405696.

I tried some other numbers and some numbers come up and some don't

Doing a quick try, 1,2,3,4,8,10 NEAR gives exact number, 5,6,7,9 doesn't. I'm investigating where and why it goes wrong

[lunap2p]

@ailisp I also tried with some other numbers with decimal value and got the same dust.
I have not found any contact with this number: 2594073385365405696
I think this bug can cause some serious vulnerabilities in future defi applications as it creates incorrect offsets

[lunap2p]

[ailisp]

Yes I feel the same, it could be some local uint128 handling bug, that's serious.

[ailisp]

Findings so far:

1. workspaces-js attachedDeposit passed correct number to the contract (by check in a equivalent rust contract)
2. C function u128_to_quickjs, produce the right result, the argument of u128_to_quickjs is debug printed by JS_NewBigUint64, which is problematic, see below
3. After C function call attached_deposit((uint64_t)ptr); is correct, *ptr is right after the call

[ailisp]

I found the reason, the C function JS_NewBigUint64 is problematic:

return JS_NewBigUint64(ctx, 0x8b0a00a425000000); // lower 8 bytes of 5000000000000000000000000
// Got 0xaf0a00a425000000 ! exactly 2594073385365405696 more than correct.

JS_NewBigUint64 is just a wrapper of bf_set_ui, so possibly bf_set_ui is wrong on some numbers. attachedDeposit writes a C u128 result and then it's convert to JS with u128_to_quickjs, which calls bf_set_ui twice to set the lower and higher 64 bit uint.

[ailisp]

Root cause is indeed bf_set_ui from libbf.c :(

Minimum repro: adding this test to builder:

void *realloc2(void *opaque, void *ptr, size_t size) {
  return realloc(ptr, size);
}

void test_bf_set_ui() __attribute__((export_name("test_bf_set_ui"))) {
  bf_t a;
  bf_context_t ctx;
  bf_context_init(&ctx, realloc2, NULL);
  bf_init(&ctx, &a);
  bf_set_ui(&a, 0x8b0a00a425000000ul);
  uint64_t ret=0;
  bf_get_uint64(&ret, &a);

  value_return(8, &ret);
}

and run it with workspaces-js, viewRaw

test.only('test_bf_set_ui', async t => {
    const { counter, ali, bob } = t.context.accounts;
    let result = await counter.viewRaw('test_bf_set_ui', {});
    t.is(result, '');
});

the number it get become 0xaf0a00a425000000. cc @volovyks

[ailisp]

Debugging bf_set_ui, It's likely quickjs is right, but our compiler option / specific #define / wasm runtime configuration went wrong. I tried value_return these things, in set_bf_ui:

LIMB_BITS - shift // => 32
a0 // => 0x25000000
a0 >> 32 // => 0. It's as expected since a0: uint32_t
a0 >> (LIMB_BITS - shift) // => 0x25000000 (!)

If a0 >> (LIMB_BITS - shift) really produce 0, then the result would be correct, so the problem is not quickjs

[lunap2p]

@ailisp I'm trying to fix it

[ailisp]

@lunap2p Do you have any luck? I did further experiment with build flags and refer to the C standard C99 and C17, the problem is (for right bitwise shift):

If the value of the right operand is negative or is greater than or equal to the width of the promoted left operand, the behavior is undefined.

And this is indeed true with current gcc and clang, and even different compiler flags of the same compiler. I tried this:

#define LIMB_BITS 32
#include <stdio.h>
#include <stdint.h>

uint32_t opaque(uint32_t x) {
  return x == 0x3;
}

int main() {
  uint32_t temp = 7;
  uint32_t x = 0x8b0a00a4;
  uint32_t k = LIMB_BITS - opaque(x) * LIMB_BITS;
  temp = temp >> k;
  printf("%d\n", k); // 32
  printf("%d\n", temp); // 7 (!)
  printf("%d\n", temp>>32);
}

Compile with gcc, gcc -Os, clang and clang -Oz (the last is the flags for compiling near-sdk-js), they gave:

~ ./repro-clang
32
7
7
~ ./repro-clangOz
32
7
36180640
~ ./repro-gcc
32
7
7
~ ./repro-gccOs
32
0
0

Therefore, it seems a mistake that quickjs developer (and I) assumed uint32_t >> 32 == 0 - it's actually an undefined behavior, therefore the usage in quickjs (LIMB_BITS - shift) , where shift is the number of leading 0s of the higher 32 bit of a uint64, which can be 0, is compiler-dependent and problematic. I'll create an issue there.

For a fix, a quick try that special case shift == 0 can fix this issue, but overall, libbf (the bigint library in quickjs) is lacking unit tests, which is a bigger trouble. We need to ensure it's reliable for finanical apps by either thoroughly test / audit by our team, or replace implementation with a more battle-test bigint library.

[ailisp]

@lunap2p I made a fix and tests for this specific issue, please have a look: #249. We'll have thorough test in quickjs to ensure it's reliability.