Forced contract version check

[chengevo]

EOSIO was designed to be able to easily update smart contract deployed on an account. While enjoying the convenience, it also introduced some troubles, some might be even dangerous.

Say Alice have deployed a token contract on the account alice, and created a token named alicetoken. Inside the contract implemented a function transfer to transfer token from an account to another:

void token::transfer( account_name from, account_name to, asset quantity,string memo )
{
    sub_balance( from, quantity );
    add_balance( to, quantity, payer );
}

it all looks good and everyone is happy.

However, suddenly Alice decided she need to take some “tax” from her beloved users, so she silently change the transfer function:

void token::transfer( account_name from, account_name to, asset quantity,string memo )
{  
    sub_balance( from, quantity );
    add_balance( to, quantity, payer );

    // pay tax to Alice
    int64_t tax = (int64_t)(quantity.amount * 0.01);
    require_auth( from );
    add_balance( alice, tax, _self );
    sub_balance( from, tax );
}

now Alice is happy but others get hurt.

This is only an example based on token transfer in a simple token contract, but this issue exists in any kind of contract deployed on EOSIO system. Recently one incident already happened in real world: Everipedia made changes to their token contract, that charges transaction fee for whoever sent IQ.

Malicious group can intentionally change the contract to attack their users, cause chaos for the community. Even if it’s a planed contract upgrade, can still cause lost if users didn’t update their program in time to adapt to the contract.

A proposed solution is to enforce contract hash check:

• while sending a action to a contract, must along sent that contract’s hash indicating which version of the contract you’re intend to talk to
• inside controller#push_transaction, before executing an action, verifies if the hash matches the contract’s hash on chain. If not, abort action execution immediately.

[chengevo]

Looking deep into code base a little bit more, I figure it's more preferable to implement this mechanism in producer_plugin#on_incoming_transaction_async, with something like following:

if( fc::time_point(trx->expiration()) < block_time ) {
  send_response(std::static_pointer_cast<fc::exception>(std::make_shared<expired_tx_exception>(FC_LOG_MESSAGE(error, "expired transaction ${id}", ("id", id)) )));
  return;
 }

if( chain.is_known_unexpired_transaction(id) ) {
  send_response(std::static_pointer_cast<fc::exception>(std::make_shared<tx_duplicate>(FC_LOG_MESSAGE(error, "duplicate transaction ${id}", ("id", id)) )));
  return;
 }

[jnordberg]

This is a sorely missed feature and It would solve a lot of problems. Not only for trust but also for clients trying to manage the encoding. Currently many APIs just assume that any action will be decodable by whatever the latest ABI is, which fails in many cases.

I think a better approach would be to use the code_sequence and abi_sequence as versions instead of a hash of the code. It would occupy much less space on the wire and chain while serving the same function.

Something like:

{
  "account": "mytrustedbank",
  "name":  "deposit",
  "version": { "abi": 22, "code": 22 },
  "authorization": [ { "actor": "bobby", "permission": "active" } ],
  "data": { "from": "bobby", "quantity": "100000.0000 EOS" }
}

[jgiszczak]

Closing in favor of #6168.

[jnordberg]

@jgiszczak These are separate issues, the one you linked is talking about accessing code versions from contracts while this one talks about specifying an expected version when submitting a transaction

[jgiszczak]

6168 enables this one. It inverts the logic, but the affect is the same. Rather than submit a transaction with an expected version, the version is checked and the transaction is not submitted if the required version is not available. Same result.

From outside a contract, the required version can already be checked for, and cheaply, since the get_code_hash API was implemented.

[jnordberg]

I don't see how would it solve the case where you want a guarantee that the transaction you submit executes on code you have verified or not at all. What if a code update happens between my get_code_hash call and the transaction execution? Or if the transaction is using delay_sec?

[jgiszczak]

Fair points. #6168 is indeed inadequate.