Origin: Meta Transactions

[micahalcorn]

This tracks the introduction of a proxy contract so that at least some users do not have to acquire ETH and pay gas to create listings and publish identities.

---

At Origin, we use ZenHub to manage our engineering tasks and product development. Download their browser extension and check out our open workspace at github.com/originprotocol/origin#zenhub.

[nick]

P0 (must-have before launch):

• [x] UI: Swap and Make Offer
• [x] UI: Deploy account proxy
• [x] UI: Deploy account proxy via relayer
• [x] UI: Toggle proxy account on/off (for testing)
• [x] Contract: Simplify salt to allow different initial tx
• [x] Contract: 'finalize + send funds to seller' convenience method
• [x] Pay with Dai when no proxy yet should approve dai to predicted proxy
• [x] UI: Toggle relayer on/off

P1 (not blocker for launch but should be done shortly after)

• [ ] DApp: send fraud data to relayer (fingerprint; desktop / mobile uid; etc...).
• [ ] UI: Add tests for OGN usage on listing

P2 (future, nice-to-have improvements)

• [ ] Contract: Require extra signature from Origin for forwarder. This is to allow to restrict meta-tx to a whitelist.
• [ ] Contract: Account upgradable? Note: It might be easier to simply deploy a new contract.

[shahthepro]

A few things I noted during my audit:

• Nonce has been hardcoded to zero, which means it is accurate only if Proxy Creation is the first transaction of the Ethereum account. If we are going to default to meta transactions in production, people may not always create a new wallet to be used in DApp.
  https://github.com/OriginProtocol/origin/blob/master/packages/graphql/src/utils/proxy.js#L20

• Do we really need the changeOwner method in IdentityProxy.s file to be public scoped?

  Say an user with wallet 0xA deploys a proxy 0x1. He can transfer the ownership of the proxy to some other ethereum address by calling changeOwner. But even after the proxy owner has been changed, We still consider that 0xA owns the proxy 0x1 (since we are predicting the proxy contract address). Also what happens to listings, offers and other things created with that proxy?

  I guess we can either change it to internal scoped or update hasProxy method to query the proxy address from GraphQL (we may need to probably store/cache it to Database/Redis and update it when OwnerChanged event is emitted on the contracts).

• I’m guessing we are probably going to restrict based on to contract address and function signature (in addition to other fraud checks), right? https://github.com/OriginProtocol/origin/blob/master/infra/relayer/src/risk/dev/engine.js#L27

• Instead of clearing the entire cache, we could just delete/update the key for which the value might have changed. Something like hasProxy.cache.delete(ethAddress). We don’t have to rebuild the cache every time when in performance mode.
  https://github.com/OriginProtocol/origin/blob/master/packages/graphql/src/utils/proxy.js#L76-L79

• Does this has to happen synchronously? https://github.com/OriginProtocol/origin/blob/master/infra/relayer/src/relayer.js#L303

[nick]

• Nonce has been hardcoded to zero, which means it is accurate only if Proxy Creation is the first transaction of the Ethereum account. If we are going to default to meta transactions in production, people may not always create a new wallet to be used in DApp.

This is not a regular nonce, but a 'proxy' nonce that is called as part of the ProxyFactory in order to determine what the proxy address will be. So a user's wallet nonce shouldn't affect it.

• Do we really need the changeOwner method in IdentityProxy.s file to be public scoped?

Right now it's not being used in the UI or GraphQL, but the idea is that a user can move control of their Proxy to another wallet in the future. In that case all their listings an offers would also move to the new account.

• I’m guessing we are probably going to restrict based on to contract address and function signature (in addition to other fraud checks), right?

Yep that's the idea for now

• Instead of clearing the entire cache, we could just delete/update the key for which the value might have changed. Something like hasProxy.cache.delete(ethAddress). We don’t have to rebuild the cache every time when in performance mode.

Sure that works.

https://github.com/OriginProtocol/origin/blob/master/packages/graphql/src/utils/proxy.js#L76-L79

• Does this has to happen synchronously?

Probably not

[shahthepro]

This is not a regular nonce, but a 'proxy' nonce that is called as part of the ProxyFactory in order to determine what the proxy address will be. So a user's wallet nonce shouldn't affect it.

Ahh, my mistake. So as of now, we are generating the address of the first deployed proxy contract, right?

Right now it's not being used in the UI or GraphQL, but the idea is that a user can move control of their Proxy to another wallet in the future. In that case all their listings an offers would also move to the new account.

Sounds good to me.

[nick]

Ahh, my mistake. So as of now, we are generating the address of the first deployed proxy contract, right?

Yea. The same user can generate multiple proxies by changing the nonce value, not that they'd need to in our case.

[DanielVF]

I could be wrong about all of these.

Proxies

I've looked at the Proxy and ProxyFactory, and have not found a real issue yet. Still going to stare hard at createProxywithSender, and the implications of changeMasterCopy.

Delegatecall is okay here

Delegatecall can be dangerous, but should be okay in the way it's used here. Any contract that you delegatecall can do whatever it wants to your contract's guts - but in this case we are always and only delegating to the mastercopy which had better be trustworthy.

ERC 725 IdentityProxy

Incompatible with modern versions of the Solidity compiler

This contract is set to compile with old Solidity 0.4.24, and cannot compile on current versions of the compilers. The actual ERC 725 interface is stuck in the past, which is why the ERC 725 proposal has updated with a version 2.

It seems awkward to have a contract we can't compile without using a different compiler, and whose function signatures can't be used unmodified in other contracts.

changeOwners

There are so multiple methods that change the contract owner as a part of normal operation. This feels like a footgun waiting to go off. It makes it easy for a normal looking function call to actually be something that irrevocably gives away your identity. I'd be happier if these methods had a one-time-use component that allowed us to one time give away a contract and perform an action, and then this be blocked from ever happening again.

swapAndMakeOffer

swapAndMakeOffer makes several external function calls without checking that they have succeeded.

ERC20 calls are not checked for success

The main ERC20 calls transfer, approve, and transferFrom return a boolean success result. We aren't checking that these have succeeded before proceeding. It's possible that we may not care in some circumstances - though that would seem unintuitive behavior - if so, we should comment why we aren't checking. Otherwise, we need some require's.

transferToOwner

transferToOwner by default, and in our normal use, can be called by anyone. This means that our identities cannot keep money in them if any malicious person wants to pay the gas to move that money out of the identity.

marketplaceFinalizeAndPay

We should add a comment that marketplaceFinalizeAndPay must only be used when the listing seller is one of our identity contracts. transferToOwner is called on the seller address, which may not be one of our identity contracts. (Our graphql code correctly checks for this, it should just also be noted in the contract code.)

Identity contract starts out world-writable

By default, when deployed, our ERC 725 identity contract starts out owned by the world until a changeOwner is called. Anyone can take the contract over or call methods through it as if they were the owner.

Now this isn't a problem as we are using it, since we call changeOwner from our proxy creation code, but this could really trip someone up who was just copying our code. We probably need to add some kind of warning in the contract comments and in our proxy initialization code https://github.com/OriginProtocol/origin/blob/63eba3096e19461cfbc77b276c4d5521ba1a5930/packages/contracts/test-alt/IdentityProxy.js#L80-L91

[franckc]

@DanielVF @nick Regarding the contract audit findings, in your opinion are there any items that are must-have before we enable the relayer by default for all transactions ? As @nick mentioned on the status call this morning, all those seems nice improvement but not must-have before the launch. And we can implement the nice-to-have later by upgrading our contract without too much trouble. Please confirm...

[nick]

Incompatible with modern versions of the Solidity compiler

Yea right now the new contracts require Solidity 0.5 and so we now have to support both 0.4 and 0.5. Since the new contracts use method only available in Solidity 0.5 they won't work with 0.4 so stuck with this for now.

changeOwner

Perhaps we could change this to changeOwnerIfNull in a future version to avoid any accidental ownership changes.

swapAndMakeOffer

Yep can update that

ERC20 calls are not checked for success

Makes sense to check

transferToOwner

Probably best the proxies don't store value anyway

marketplaceFinalizeAndPay

Sure can add comment

[DanielVF]

@franckc I think the priority ones are adding the require's to the ERC20 actions and to swapAndMakeOffer.

[franckc]

@nick Do you think you could add those requires to the contract before we enable meta-txn by default ? Talking to Micah and Tom, looks like the timeline would be:

• This week: enable meta-txn by default in DApp and publish the new mobile app to app store
• Monday July 8th: public announcement

[micahalcorn]

Considering this done 👏