Add regaccount function

[elmato]

This new regaccount function allows users to claim their unregistered EOS Accounts using their Ethereum's private keys.

[wanderingbort]

I think this message is insufficient. Unless I've missed some other secure commitment to it, this allows an attacker to copy the signature parameter and use the same LIB/tapos information to forge a competing claim that would route funds to a different account and/or eos-pubkey.

at a minimum this message should include the eos-pubkey. I think it should include the account name as well but so long as the rightful owner has control that is less of an issue.

[arhag]

Although it shouldn't make much of a difference as long as the connector weights for both connectors are equal, the 1000.0 factor here is no longer consistent with what is used in the eosio.system contract since v1.1.0. So to reduce errors in what this contract estimates and what the actual system contract calculates, this line should probably be changed to real_type F(c.weight);.

[arhag]

Similarly, if you make the change described above in convert_to_exchange you should also change this line to real_type F(1.0/c.weight);.

[arhag]

There should be a check to see if balance is greater than or equal to asset(1000). Otherwise this function will return negative values for split_net and/or split_cpu. And while this will eventually cause the transaction to fail as it tries to eosio::delegatebw with negative amounts, the contract can provide a more meaningful error message if it asserts that the balance amount is sufficient here, or even simply returns {}.

[elmato]

Thanks @arhag, the minium EOS in the unregd account table
is 1 EOS, anyway we will add that assert at the begining.

[arhag]

You could also just stack allocate these small-sized buffers for slightly better performance without risk of stack overflow (especially since these functions aren't recursive).

[arhag]

As @wanderingbort mentioned, it is critical for the digest to commit also to the public key and ideally the account as well. Otherwise an attacker could potentially steal the funds of a victim who was in the process of registering using unregd:regaccount.

Also, have you considered using a struct to store tapos_block_num(), tapos_block_prefix(), naccount, and eos_pubkey, then serializing that struct into a buffer which is then sha256 hashed to give you the digest? That way you can remove this dependency on sprintf which would reduce contract size.

[arhag]

I feel uneasy that this contract is designed in a way that requires the eosio.unregd account to be privileged for it to be able to operate. It is best to reduce the attack surface of privileged code to just what really needs to be privileged (within additional reasonable constraints of performance requirements and maintainability).

As far as I can see, the only reasons that this contract needs to be deployed on a privileged account are:

1. So eosio can be the one that created the account (but why not just have eosio.unregd be the one to create the account?).
2. So that the contract can issue new EOS tokens into existence and use it to buy 8 KiB of RAM for the new account.

(Note: it is also possible to avoid making eosio.unregd privileged and still allowing this contract code to function, but it requires allowing eosio.unregd@eosio.code to unilaterally satisfy the active authority of eosio, which still comes with the risk that a full compromise of the eosio.unregd contract, as unlikely as it may be, could change the contract code of the eosio account and thus escalate privileges anyway.)

Regarding reason 2, I personally question the decision to allow the contract to issue into existence a practically unbounded amount of EOS (because the contract pays whatever it costs to buy 8 KiB of RAM regardless of what the price of RAM may be at the time). Nevertheless, if block producers do wish to continue with that policy, there are other ways of doing it that provides greater safeguards and allows the eosio.unregd account to remain unprivileged.

BPs could create a new account, say eosio.regram, whose active authority is also unilaterally satisfied by the eosio.unregd@eosio.code permission level. Then instead of issuing the necessary EOS within the action, the contract can simply buyram using the eosio.regram account. BPs would first need to load up the eosio.regram account with sufficient EOS, likely by issuing it or maybe pulling it from existing accumulated fees (I wonder if it needs referendum approval either way?).

But by doing it in this indirect way, if something went wrong with the contract and it calculated a very large amount of EOS to use to buy the RAM, the damage would be limited to the amount of EOS kept in the eosio.regram account.

The contract could be made even more robust with a further safeguard which adds an assertion within the unregd::regaccount action to limit how large the amount_to_purchase_8kb_of_RAM value can be. Another action could be added to the contract (that requires the authority of _self) to change that limit, so that BPs could explicitly adjust it as necessary without needing to update the whole contract.

[arhag]

Here is the alternative approach to avoid using string operations and parsing the public key string:

1. Generate the msghash using the SHA256 hash of the raw packed data of the tuple (tapos_block_num(), tapos_block_prefix(), new_pubkey, account) where the account (now of account_name type rather than string type) would be the second argument of the regaccount action and new_pubkey (of eosio::public_key type) would be the third argument of the regaccount action.
2. Convert the first 30 bytes of msghash data into a Base64 encoded string (which should consist of exactly 40 ASCII characters).
3. memcpy into the beginning of a char message[68] the first 28 bytes of the string "\x19Ethereum Signed Message:\n40" and then memcpy into message + 28 the first 40 bytes of the Base64 encoded string generated in step 2.
4. Then calculate the SHA3 hash of message similarly to what was done before: rhash_keccak_update(&shactx, (const uint8_t*)message, sizeof(message));.

Steps 1 and 2 would have to be replicated in an open-source client-side tool (and since it has to be easier for regular users to use it would probably have to be implemented in Javascript and/or Python) so that users can simply input their newly desired account name and the new public key (in convenient text format) into the tool which would generate the 40 character Base64-encoded string. The tool would then pause (keeping track of the state of inputs and the TaPoS information it selected) waiting for the signature as input. The 40 character Base64-encoded string would then be signed by the user using their appropriate Ethereum private key and any compatible Ethereum message signing tool before then inputting the outputted signature back into tool to finalize the process by signing and broadcasting the transaction containing the regaccount action.

The main downside with this approach is that the tool could substitute its own public key (say that of an attacker who tricked users into using their malicious client-side tool) as the input into step 1 and generate a Base64-encoded string that the user would have no reason to suspect has been tampered with. This risk can be mitigated by reminding users to only use client-side tools that have been audited (since the source is open) by multiple people they trust. But the good thing about the current code's approach over this alternative approach is that the user can see the public key and account name that they are putting into the Ethereum message signing tool, so they are less likely to get tricked into giving up their funds to an attacker. The disadvantage of the current code's approach is that it bloats the contract.

The second downside of this approach is that if the client-side tool needs to be implemented in Javascript or Python, more work is required to replicate the account_name conversion and public_key_type serialization functionality to accomplish step 1, because the convenient C++ library functions available in the eos repository code that would handle this stuff are not applicable to tools written in languages other than C++.

[arhag]

Latest changes look good to me. However, one minor quibble is that I'm not a fan of using string operations (sprintf, substr, etc.) in smart contracts which bloats the size of the smart contract. And now instead of using a binary encoded eosio::public_key, regaccount now takes the string of the text-encoded version of the public key and parses it in the contract, which is a little upsetting to see.

But I suppose the purpose of all this code bloat is to make it possible for users to type in human-understandable ASCII text (<first_number_returned_by_tool_that_seems_arbitrary_to_the_user>,<second_number_returned_by_tool_that_seems_arbitrary_to_the_user>,new_public_key_of_user,new_account_name_of_user) into a standard Ethereum signing tool and generate the signature that acts as the first argument to the regaccount action, right?

And I suppose that is a noble goal since the alternative, which would simplify the smart contract and reduce the WASM code size, is to avoid string operations altogether. I wrote in the review comments how the alternative could be implemented, but it comes at the disadvantage that more work needs to be done on the client-side and that code would have to be audited or trusted by the user since it could switch out the public keys in the background.

[abourget]

Really great to see that come to fruition! Thanks guys!