Feedback on dai escrow contract that reimburses a relayer using Uniswap

Hi @VypoMouse,

Welcome to the community forum :wave:

The forum is for discussions on smart contract development, so this is definitely the right place.

I recommend that smart contracts are appropriately tested and audited. OpenZeppelin offer security audits, see https://openzeppelin.com/security-audits for details.

I would also recommend reading through the checklist before an audit.

:warning: The following is my opinion as a Community Manager having a quick look. (This isn’t a review nor an audit).

  • The Solidity documentation includes an example Escrow contract (though this has different incentives to yours) which might be worth looking at for ideas: https://solidity.readthedocs.io/en/v0.6.6/solidity-by-example.html?highlight=escrow#safe-remote-purchase
  • I didn’t see any tests in the repository.
    I would recommend writing unit tests to test all the functionality: I would suggest looking at the following guide: Test smart contracts like a rockstar
    Also: https://docs.openzeppelin.com/learn/writing-automated-tests
  • You have rolled your own meta transactions (which may have been part of your own learning exercise).
    You may want to consider using the Gas Station Network. (see https://www.opengsn.org/)
    This could simplify the core of your contract to just focus on the escrow.
  • I would add documentation (such as a README) explaining how the escrow works. (you could even show a badge with your test coverage).
  • You have hardcoded DAI and UniSwap addresses in the contract. You may want to pass these in to the constructor so that you can change depending on what network you deploy to.
  • My preference is not to include self destruct or any functionality in a contract only used for testing to ensure that this doesn’t make it into production.