SmartTokenLabs/TokenScript

numeric string type ASN.1 is not checksummed on address

bitcoinwarrior1 opened this issue ยท 5 comments

observe the following:

<ts:selection id="notAdmin" filter="!(ownerAddress=admin)">
    <ts:denial>
        <ts:string xml:lang="en">Must be the admin</ts:string>
    </ts:denial>
</ts:selection>

With the following admin attribute:

<ts:attribute-type name="admin" syntax="1.3.6.1.4.1.1466.115.121.1.15">
    <ts:label>
        <ts:string xml:lang="en">admin</ts:string>
    </ts:label>
    <ts:origins>
        <ethereum:call function="admin" contract="GFO" as="address"/>
    </ts:origins>
</ts:attribute-type>

This comparison will not work because the attribute type admin is not checksummed but the owner address is, thereby making this selection impossible.

We need an ASN.1 syntax which handles checksums, not only for this use case but in general as it is unsafe NOT to use a checksum on an address.

There are 2 ways to solve this problem:

๐‘Ž) relax the filter expression rule implementation to allow 0xโ€ฆ integer
๐‘) introduce typing to attributes (therefore onboarding caseIgnoreMatch matching rule.)

The first, ๐‘Ž) is a quick fix. Let's define the syntax of admin to be Integer ( 1.3.6.1.4.1.1466.115.121.1.27 ) and relax the search expression to allow hex, therefore an owner_address address 0xdecafbad can be used in the expression (!(ownerAddress=admin)).

The disadvantage is that this is not applicable to some other blockchains (e.g. Bitcoin), or, when Ethereum upgrades to include a new address coding method like Bitcoin did when it upgraded to the bech32 format (stuff like bc1qar0srrr7xfkvy5l643lydnw9re59gtzzwf5mdq). This will involve a change in every existing TokenScript where an attribute represents an address (not much) and make the expression evaluation bits in ฮฑW ios/Android flexible (which @JamesSmartCell is doing on his side already.)

The second ๐‘) is to implement the typing of attributes which was originally intended for schema/06 but missed the deadline. It will involve changing every existing TokenScript files but I'd rather do this in 2020/06 than in 2021/06 (this is not something I come up suddenly - I wanted to propose this change ever since Boon recommended renaming <attribute-type> to <attribute>.

https://community.tokenscript.org/t/attribute-typing/368

What I propose is to do a full ๐‘Ž and a half-way ๐‘:

Convert this:

<ts:attribute-type name="admin" syntax="1.3.6.1.4.1.1466.115.121.1.15">
โ€ฆ

Into this:

<ts:attribute name="admin">
   <ts:type><ts:syntax>1.3.6.1.4.1.1466.115.121.1.27</ts:syntax><ts:type>
โ€ฆ

Rational:

  1. This avoids implementing <ts:equality>, therefore it's only a syntax change in the codebase, none of ฮฑW ios or android needs to change the way attributes are stored and handled. Easy to make it into 2020/06

  2. When some day we properly decided what to do with the typing of attribute, we don't have to force any change of existing tokenscript, only that the new tokenscript should have additional elements.

hboon commented

I think the original snippet which @James-Sangalli posted might already work in the iOS version. Does it, James?

In the iOS implementation, every (attribute) value, wherever possible, is already associated with a "type", be it the syntax or the as). We should have to do something like this in order to implement this anyway:

๐‘) introduce typing to attributes (therefore onboarding caseIgnoreMatch matching rule.)

There is this bit of code in iOS:

func isTrueFor(attributeValue: AssetInternalValue, value: String) -> Bool {
    switch attributeValue {
    case .address(let address):
        switch self {
        case .equal:
            return address.eip55String.lowercased() == value.lowercased()

Because the implicit value ownerAddress is injected/interpolated as an "address" (actually Ethereum address for now), the comparsion operator "=" compares them case-insensitively.

Would this be expected behavior as well as easier to implement right away and closer towards the path of (๐‘), if Android is already tagging types โ€” without thinking and enforcing too much type conversion and without requiring any change to the TokenScript files and schema.

If so, that's my (c)

I think the original snippet which @James-Sangalli posted might already work in the iOS version. Does it, James?

In the iOS implementation, every (attribute) value, wherever possible, is already associated with a "type", be it the syntax or the as). We should have to do something like this in order to implement this anyway:

๐‘) introduce typing to attributes (therefore onboarding caseIgnoreMatch matching rule.)

There is this bit of code in iOS:

func isTrueFor(attributeValue: AssetInternalValue, value: String) -> Bool {
    switch attributeValue {
    case .address(let address):
        switch self {
        case .equal:
            return address.eip55String.lowercased() == value.lowercased()

Because the implicit value ownerAddress is injected/interpolated as an "address" (actually Ethereum address for now), the comparsion operator "=" compares them case-insensitively.

Would this be expected behavior as well as easier to implement right away and closer towards the path of (๐‘), if Android is already tagging types โ€” without thinking and enforcing too much type conversion and without requiring any change to the TokenScript files and schema.

If so, that's my (c)

@hboon while that may be the case, the admin attribute doesn't resolve in time for the selection logic so it doesn't work right now. The admin address is resolved correctly and does do the checksum.

admin

I think the original snippet which @James-Sangalli posted might already work in the iOS version. Does it, James?
In the iOS implementation, every (attribute) value, wherever possible, is already associated with a "type", be it the syntax or the as). We should have to do something like this in order to implement this anyway:

๐‘) introduce typing to attributes (therefore onboarding caseIgnoreMatch matching rule.)

There is this bit of code in iOS:

func isTrueFor(attributeValue: AssetInternalValue, value: String) -> Bool {
    switch attributeValue {
    case .address(let address):
        switch self {
        case .equal:
            return address.eip55String.lowercased() == value.lowercased()

Because the implicit value ownerAddress is injected/interpolated as an "address" (actually Ethereum address for now), the comparsion operator "=" compares them case-insensitively.
Would this be expected behavior as well as easier to implement right away and closer towards the path of (๐‘), if Android is already tagging types โ€” without thinking and enforcing too much type conversion and without requiring any change to the TokenScript files and schema.
If so, that's my (c)

@hboon while that may be the case, the admin attribute doesn't resolve in time for the selection logic so it doesn't work right now. The admin address is resolved correctly and does do the checksum.

admin

@hboon flag this, it is probably me now that I think of it because the enabled selection resolves in compound