Identity: v5.2: optimizations and SC signature support

[Ivshti]

We need a round of improvements to the Identity/IdentityFactory system

End goals

• Security: To support the 2/2 security model, which makes the email/pwd security setup way better and provides pwd recovery; can support 2FA and email confirmation too DONE: through QuickAccManager
• dApp support: To support Opensea together with the 2/2 security model (signing msgs) DONE: isValidSignature, support for EIP721
• To support multisigs down the line, namely signatures for them (EIP 1271); DONE: isValidSignature plus SmartWallet sig type
• To make deployment cheaper (EIP 1167) DONE: tests show ~60k deployment overhead vs 158k in the old setup
• To show the contract code properly on Etherscan (EIP 1167) DONE: see this
• Better signing UX: to allow signed type data (EIP 712) through an extension contract; DONE: through QuickAccManager
• ADDED: NFT support. Turns out EIP 721 required extra code to work

End goal: deploy & make it work with opensea through WalletConnect; on Polygon too

To be implemented

• [x] Identity: sign all txns as a single batch
• [x] SignatureValidator: support EIP1271: isValidSignature: see https://github.com/gnosis/safe-contracts/blob/2620a21c0844f23df39ea98438b82e378bb334f0/contracts/GnosisSafe.sol#L255-L285 and https://samczsun.com/the-0x-vulnerability-explained/ and https://github.com/gnosis/safe-contracts/blob/186a21a74b327f17fc41217a927dea7064f74604/test/integration/GnosisSafe.0xExploit.spec.ts ; SMART_CONTRACT signature type; rename trezor to TREZOR_LEGACY; actually, drop trezor legacy
• [x] Identity: nonce optimization (increment only once before running the txns)
• [x] Identity: remove feeToken/feeAmount, we'll rely on a separate txn inside the bundle that we'll check for on the relayer
• [x] Identity: hash in chainID to prevent replay attacks
• [x] Implement https://eips.ethereum.org/EIPS/eip-1167 for lighter proxies; see https://github.com/optionality/clone-factory too
• [x] JS lib update: implement the minimal proxies in there, signing, etc.
• [x] factory, or an extra contract to support the 2/2 security mechanism
• [x] factory to support batch calls so that the relayer can try to batch multiple user bundles - no need, we'll do that through an Identity account (very meta!)
• [x] For security reasons, all of the following functionalities should be pausable until battle tested: the 2/2 mechanism, EIP712 signatures support, multisigs; DECIDED NOT TO DO THIS because the contract is simple enough to make sure it does not have vulnerabilities
• [x] Maybe: factory to support burning gas token and coinbase payments - no need, we can just directly call free on the gasToken from relayer's Identity and tipMiner

[Ivshti]

Work in progress decisions & research

• [x] should we support multiple sigs encoded in the same bytes? RESOLVED: probably not, we can do an array of bytes if we need that for the MultiSig contract; or use a different sig validator for it
• [x] legacy trezor sigs? RESOLVED: no, this has been fixed a long time ago: https://github.com/trezor/trezor-mcu/commit/1f470cf1f18de0235675cf71091bba7bf4fb762f#diff-12950855dc4a2a268fb19ece6da37668R632
• [x] how would we encode SC signatures considering they need to be arbitrary length
• [x] will Etherscan recognize EIP1167 proxies if the deploy code contains a couple of SSTORES; RESOLVED: it should, as this change is only in the deploy code, not in the contract code
  • it does, see example: https://ftmscan.com/address/0x11c9d5e254f116ff50b7224c2cbafd9a2806ba00#readContract
• [x] rather than having separate execute methods depending on whether we call directly or not, we can just add Signature type Caller; 0x had an interesting exploit with that https://github.com/ConsenSys/0x_audit_report_2018-07-23#32-mixinsignaturevalidator-insecure-signature-validator-signaturetypecaller, but it's not applicable in our case, as we require signees to have privileges RESOLVED: will do it
• [x] sigMode: encode as a separate arg, end or beginning; RESOLVED: excluding separate arg cause it wastes calldata bytes; but try to see if it's better in terms of gas usage; end makes more sense cause it's easier/cheaper to trim off when passing to isValidSignature
• [x] sig order: RESOLVED: going with {r}{s}{v} as that's the openzeppelin and more common in the industry
• [x] should we combine the factory with the other functionalities like 2/2 security to save gas and make it simpler; RESOLVED: no, separate, minimal contracts for everything is the simplest; we can compose them by having the relayer use an Identity itself to batch operations (and also do miner tips, gas token burns); even deployAndExecute won't be needed, as we can simply batch a safeDeploy + execute/magicAcc.send together
• [x] how to call the 2/2 security; RESOLVED: temporarily calling it a MagicAccount, may start using QuickAccount to be consistent with previous terminology
• [x] should we store extra data in privileges[] to avoid using an extra storage slot when setting up a MagicAccount; RESOLVED: yes, it saves us 20k gas during deployment; allows us to store data for other account managers in the future, such as MultiSigAccManager; to minimize the chance of bricking, we recommend that this is modified only directly via the acc manager calling executeBySender() with setAddrPrivilege, so that it doesn't set a gibberish value or simply true

[Ivshti]

How to generate min proxy code

sstore code: {pushVal}{val}7f{slotHash}55,

0x{sstoreCode}3d602d8060{codeOffset}3d3981f3363d3d373d3d3d363d73{masterContract}5af43d82803e903d91602b57fd5bf3

example

> require('./js/IdentityProxyDeploy').getMappingSstore(0, 'address', '0xe5a4Dad2Ea987215460379Ab285DF87136E83BEA','0x01')
'sstore(0x02c94ba85f2ea274a3869293a0a9bf447d073c83c617963b0be7c862ec2ee44e, 0x01)'

starting with deploy code 0x3d602d80600a3d3981f3363d3d373d3d3d363d73bebebebebebebebebebebebebebebebebebebebe5af43d82803e903d91602b57fd5bf3 (taken from https://github.com/optionality/clone-factory/blob/master/contracts/CloneFactory.sol#L32 )

then 6001 to push the val (push1 0x01), followed by 7f{slotHash}55 
replace bebebebebebebebe
finally, 60e2 instead of 600a to adjust the start of the code 

0x60017f02c94ba85f2ea274a3869293a0a9bf447d073c83c617963b0be7c862ec2ee44e553d602d80602e3d3981f3363d3d373d3d3d363d7302a63ec1bced5545296a5193e652e25ec0bae4105af43d82803e903d91602b57fd5bf3

[Ivshti]

Decided against specific anti-bricking mechanisms in QuickAccManager

The reason for this is that code becomes complicated with two separate schedule/execscheduled routines

We can employ off-chain anti bricking by tracking scheduled txns; if one doesn't make sense (eg no adex control after it), cancel it; we have to simulate executeBySender() call with all the txns