daimo-eth / daimo

Dollars, anywhere in the world
https://daimo.com
GNU General Public License v3.0
372 stars 32 forks source link

Audit followup #261

Closed dcposch closed 1 year ago

dcposch commented 1 year ago

Questions from Veridise

Is it intended behavior for only one ephemeral note to exist for a given user at any point in time?

An ephemeral note can be issued to a smart contract, but it is impossible for a smart contract to claim an ephemeral note as it cannot sign a message.

(Explain Payment Links)

Are ephemeral notes meant to be used with real people (EOAs), or are they also meant to be used with smart contracts (e.g., account abstraction wallets)?

Neither.

What is the intention behind the nonce generation scheme in daimo-userop?

  1. Ability to pass a bit of extra information, eg request ID
  2. Non-sequential nonce allows offline payment

Why does P256 verifier reject points of the form (0, y) or (x, 0)?

(Discuss P256 curve math)

Is the intended use case that new faces (for FaceId) and new fingerprints (for TouchId) be allowed to unlock the enclave?

Yes

Why are private keys (and representations thereof) stored in the KeyChain marked as generic passwords (with kSecClassGenericPassword) and not keys (kSecClassKey)?

From @nalinbhardwaj : couldn't see a difference between the two. What we care about is that it's stored encrypted on disk, which both should be. (Either way, this applies to the fallback when secure hardware is unavailable.)

https://developer.apple.com/documentation/security/ksecclasskey https://developer.apple.com/documentation/security/ksecclassgenericpassword

Some other issues we are still investigating

Comments in DaimoNameRegistry indicate that it is upgradable, but it uses the standard OpenZeppelin contracts instead of the upgradable contracts. This may result in problems during initialization.

It does implement Initializable. Do we need anything else for use with TransparentUpgradeableProxy?

Also, given the OZ no longer recommends TransparentUpgradeableProxy, we may switch to UUPSUpgradeable.

DaimoNameRegistry does not disable initializers in its constructor.

Good call, will add.

The DaimoAccount signature format does not include any timestamps, so signatures last forever. This could increase the risk of replay attacks, especially given that daimo-userop constructs nonces that always have a sequence value of 0.

Will add.

Reply attacks should not be possible since the signatures are tied to a specific nonce and chainID. However, a bundler could fail to include a transaction, and then include it at an indefinite point in the future. Time-bounded signatures should fix that.

dcposch commented 1 year ago

done, see #277