Misleading behavior of encoding string into Fr

[Groxan]

Hey! I've faced an issue with encodeArguments function - when it tries to encode string into Fr it uses Fr.fromString, which always treats the input as a hex string. Even though I understand the rationale, this behavior is quite arguable, because it can be dangerous in some scenarios.

Imagine, a dAPP is sending an execution request to a wallet via walletconnect or something, for example, to call transfer(to, amount) function with arguments ("0x12..34", "100"), and a user is asked for confirmation. So, the user will check the args of the call and will see "100", and will mistakenly think that he is approving a transfer of 100 tokens. But "100" will actually be treated as hex and will be encoded into 256, so actually 256 tokens will be transferred. So, this behavior can not only mislead users, but also be exploited by scammers.

It would be cool if either ArgumentEncoder.encodeArgument or Fr.fromString treated input strings as hex only if it started with 0x, so that "100" is encoded into 100 and "0x100" into 256. This would be more intuitive and will help to avoid bad UX.

[olehmisar]

Related discussion. #7365 (comment)

@Maddiaa0 were you able to take a look at this? I hit this "feature" too many times already and I saw people get bitten by this issue multiple times.

[Maddiaa0]

Related discussion. #7365 (comment)

@Maddiaa0 were you able to take a look at this? I hit this "feature" too many times already and I saw people get bitten by this issue multiple times.

Yep, I've discussed it and think it'll take higher priory to change now, imo fromString for buffer like types should change to fromHex and fromString as listed here should check encoding.

[Maddiaa0]

#10622

Commenting to link the issues

[Maddiaa0]

fixed by #10529

[Groxan]

@Maddiaa0 sorry, but the issue hasn't been fixed.
In order to fix it, this line:

else if (typeof arg === 'string') {
    this.flattened.push(Fr.fromHexString(arg));
}

should be changed to this:

else if (typeof arg === 'string') {
    this.flattened.push(Fr.fromString(arg));
}

That's it :)

[sklppy88]

@Maddiaa0 sorry, but the issue hasn't been fixed.
In order to fix it, this line:

else if (typeof arg === 'string') {
    this.flattened.push(Fr.fromHexString(arg));
}

should be changed to this:

else if (typeof arg === 'string') {
    this.flattened.push(Fr.fromString(arg));
}

That's it :)

Ah yep, the native behavior has been changed, but it needs the encoder as well, good eye. Will push a fix asap. Thanks for the comment !