ERC-20: forbid using invalid NEP141 AccountID for mapping

[sept-en]

Fixes #173.

[birchmd]

The code itself looks good, but I'd like to understand the context here.

It was mentioned on the Monday call that this is meant to fix a phishing attack where someone tries to spoof a token using different capitalization. But I thought @mfornet said this attack wouldn't be possible if the front-end interfaces were sufficiently good.

It's also unclear to me what this attack would accomplish. Could you spell out in more detail what could happen from a user's perspective?

[sept-en]

@birchmd it's not only about the attack itself (which needed to be really complex and I don't expect this to happen in the nearest future), but for the code consistency and right conceptual structure.
Without this fix deploying some address: 123abc..cba321.factory.bridge.near and 123Abc..cba321.factory.bridge.near (one letter is uppercase) will deploy 2 ERC-20 tokens for the same NEP-141 address. So we might end up with many different ERC-20 tokens deployed for the same NEP-141 account.

[birchmd]

Oh, NEAR account IDs must be lowercase anyways, so it makes sense that we always ensure lowercase is used.

[sept-en]

@birchmd yes, exactly. I guess this was explicitly made to eliminate even the possibility of such issues. For our case, it's expected that we can receive this as an input because users may specify ERC-20 checksummed address which contains both upper- and lower- case letters. But we need all those versions to point to the same ERC-20 on Aurora.

Created #180 for the purpose of checking the validity of all used AccountIds

[joshuajbouw]

Still in draft, do you want that review?

[joshuajbouw]

In general, there needs to be better data handling that doesn't involve copying borrowed data first.

[sept-en]

@birchmd @joshuajbouw @mfornet
Gents, please check the update! This contains custom error types and forbids providing invalid NEP-141 AccountID.

[birchmd]

LGTM

I wonder if it would be nicer to validate the input closer to the boundary of the system and not have errors bubbling up from core logic. I think the PR is ok as-is, but just something that I thought I'd mention to see what others think.

With the aforementioned model the code would look something like

/// Can only be constructed with `ValidAccountId::from_bytes` which does validation.
/// Therefore if a function takes `ValidAccountId` it does not need to worry about invalid accounts.
struct ValidAccountId<'a>(&'a [u8]);
impl ValidAccountId {
    fn from_bytes(bytes: &[u8]) -> Option<Self> {
        if is_valid_account(bytes)  { Some(Self(bytes)) }
        else { None }
    }
}

pub fn register_token(&mut self, erc20_token: &[u8], nep141_token: ValidAccountId) { ... }
pub fn get_erc20_from_nep141(nep141_token: ValidAccountId) -> Option<Vec<u8>> { ... }

// lib.rs
fn deploy_erc20_token() {
    ...
    let nep141_token = ValidAccountId::from_bytes(&args.nep141).sdk_expect(ERR_INVALID_NEP141_ACCOUNT_ID);
    engine.register_token(address.as_bytes(), nep141_token).sdk_unwrap();
    ....
}

[sept-en]

@birchmd thanks, this is a good suggestion! I wanted to do something similar in the scope of PR that will fix #180.
I agree with you as the model you mentioned will simplify the code, its usage, and readability.

[joshuajbouw]

@sept-en when you get back, please fix the conflicts. Thanks buddy.