aeternity / protocol

Specification of the æternity blockchain protocol
https://docs.aeternity.com/protocol
208 stars 76 forks source link

Update generalized account example #484

Closed cytadela8 closed 1 year ago

cytadela8 commented 3 years ago

In generalized accounts code one needs to check the gas (cost) and fee to avert rouge miners and some other attacks.

We could use some advanced generalized accounts examples. (eg multi signature wallet)

marc0olo commented 3 years ago

@cytadela8 Is this a check the developer needs to take care of? Can you elaborate a bit more about the problem?

Asking because @mitch-lbw is implementing a multsig example for generalized accounts.

hanssv commented 2 years ago

@marc0olo any follow up on this?

marc0olo commented 2 years ago

I think I didn't really understand the problem back then and never had a deep look. afaik @mitch-lbw is about to finish the simple usecase for MultiSig using generalized accounts.

not sure what should be updated in the docs.

mitch-lbw commented 2 years ago

@cytadela8 can you please explain the issues you see? I currently unfortunately cannot retrace your considerations since interacting with GA is still contract interaction und thus is validated concerning gas and fee consumption ...

hanssv commented 2 years ago

So this is what I recall from this discussion...

Remember that for a GAMetaTx the signature is, in effect, the calldata; and the signature check is the authentication function. So a malicious miner could change the Fee and the GasPrice and thus get more reward for the transaction - to counter this you'd have to check that Fee and GasPrice are sensible in the authentication function (either by having a pre-set range or by adding them as arguments to the authentication function).

marc0olo commented 2 years ago

I think I still don't 100% understand this. does this mean that the Fee and GasPrice set for the GAMetaTx (e.g. SpendTx) are irrelevant for the execution of the auth function?

from the solution point of view I would prefer providing this information in the arguments of the auth function.

@hanssv can you share a small example how this would look like for a SpendTx for example? just wondering

hanssv commented 2 years ago

GAMetaTx contains a fee for the Meta part, and a fee for the inner tx (e.g. SpendTx). The inner tx is hashed and the hash is available to the authentication function. The outer fee (and gas price) has to be protected explicitly - this could look something like:

contract GAAuth =
  stateful entrypoint authorize(fee: int, gasprice: int, txhash: hash, ...) : bool =
    require(fee == Call.fee, "Fee has been tampered with")
    require(gasprice == Call.gas_price, "Gas preice has been tampered with")
    require(Some(txhash) == Auth.tx_hash, "Inner transaction has been tampered with")
    ...
marc0olo commented 2 years ago

ok so I think the docs should just outline that there is this kind of attack vendor, right?

when I understand right the following check would generally be sufficient to ensure the correct fee and gasPrice of the inner tx is being used:

require(Some(txhash) == Auth.tx_hash, "Inner transaction has been tampered with")

hanssv commented 2 years ago

Remember that for a GAMetaTx the signature is, in effect, the calldata; and the signature check is the authentication function. So a malicious miner could change the Fee and the GasPrice and thus get more reward for the transaction - to counter this you'd have to check that Fee and GasPrice are sensible in the authentication function (either by having a pre-set range or by adding them as arguments to the authentication function).

Note to self: Adding them as arguments to the authentication function is just making the work of the malicious miner a bit harder. Nothing stops the miner from changing the AuthData in the call.

hanssv commented 1 year ago

This is addressed by #513 - and in Ceres this is handled at the protocol level.