AssetManager Contract

[johnwhitton]

This first PR is at a logical point to be Merged the Todo items and clarifications have been documented in Issue #3 and can be worked on separately.

ToDo

• Add in support for ERC777. References ERC-777 Sample Code, EIP-777, OpenZepplin ERC777 Documentation, OpenZepplin ERC777.sol, OpenZepplin ERC777 Example Implementation, ERC777 Masters Thesis
  • Initial code is was done in commit b467f85 on branch jw-erc-777
• Testing
  • Additional tests for require, modifiers and pausable functions
  • Additional tests when adding operators over thresholds
  • Additional tests for depositing or approving funds over global limits
  • Refactor testing
• Documentation: add how to run coverage server and
• Deployment
  • Review deployment best practices for proxies including using hardhat-deploy instead of openzepplin upgrades
  • Deploy to Testnet
  • Verify contract on Testnet
  • Test Contract Upgrade Process
• Enhancements
  • Replace all requires with if tests reverts and custom errors
  • Could introduce warning events when changing thresholds if user has a balance over the new threshold
• Optimizations
  • Review contract optimization best practices
• Security
  • add yarn.lock to gitIgnore comments here

Outstanding Issues/Clarifications

• ERC721 safeTransferFrom not working in AssetManager.ts (using transferFrom)
• ERC1155 safeTransferFrom needs "0x" passed for data in AssetManager.ts ("" in AssetManager.sol)
• Review errorHanding for transfer (currently capture failures for ERC20 but not ERC721 and ERC1155).
• Deploy failing with unpredicatable gas limit was caused when there was a bug in the contract (not setting the admin role before setting operators) if this happens again may need to apply this workaround to fix gas price

Done

• Add Transfer Events
• Transfer function
• Index events
• User errors and revert (rather than events and require)
• Add operator (responsible for day to day operations)
• Fix bug with ERC20 checking of approvals
• Make AssetManager Upgradeable (owner function)
• Make AssetManager Pausable (owner function)
• Add user limits for destination addresses (not just a global limit)
• Add global per-user mini-wallet limit(owner function)
• Testing cater for gas usage when checking balances
• Ability to limit deposit amount
• Review Auth logic (updating global limit and ensure it is checked when setting auth)
• Withdraw logic should this decrease the allowance (@polymorpher Should authorize the difference, not newBalance) (@johnwhitton should not reduce authorization unless balance is below authorization as withdraw should not reduce send limit)

Current Logic
        uint256 currentLimit = this.userAuthorizations(address(msg.sender));
        if (currentLimit > newBalance ) { authorize(newBalance); }

examples
1. If they have deposited 5 Native Tokens and authorized 2 Tokens then withdraw 4 tokens then we set authorized 1.
2. If they have deposited 5 Native Tokens and authorized 3 Tokens then withdraw 1 token currently we leave the authorization at 3 Tokens (rather than reducing it to 2 tokens).

Decision: withdrawal logic does not effect authorizations (logic above removed)

• AssetManager comments
• Review Reentrancy guard not needed
• review PullPayment not needed

Clarifications also added in the SMS Wallet Design Document

• withdraw: reduces the amount authorized by the amount withdrawn
• withdraw: Withdraws all user funds if a zero value is passed
• send: Reduces the amount authorized by the sent amount
• send: can only called by the operator of the AssetManager Contract (users will use withdraw and then send the native tokens by signing transactions themselves)
• transfer: users will transfer the ERC20/721/1155 tokens to the AssetManager contract and then the owner of the AssetManager contract can send these tokens on the users behalf.
• transfer: Users can approve the ERC20/721/1155 tokens using approve function and then the owner of the AssetManager contract can send these tokens on the users behalf.
• Ensure we are updating and checking operatorThreshold when adding and removing operators
• Ability to Retrieve Operators
• Remove redundant configuration variables from .env, .env.example and config.js
• Modular Testing
• Test cases for Operator Admin
• Test cases for Pausing contract
• Negative use case testing
• Testing replace console.logs with expects
• Reach 100% coverage for AssetManager.sol
• Document coverage and gas usage

[polymorpher]

We can remove all boilerplates and auto-generated files. This includes docs/, elin/, scripts/, data/abi. Prettier is inconsistent with eslint - I suggest we remove prettier for now. README and some other files seem to be boilerplates - can they be removed until needed, or minimized to essential elements only?

[polymorpher]

Need to be access-controlled and respect limits

[johnwhitton]

I don't think this needs access control (anyone can approve amounts for themselves (msg.sender)).
Will add in limit check.

[polymorpher]

I think we will need multiple operators to allow the server serving multiple SMS commands at the same time. This is similar to the problem with relayer in https://github.com/polymorpher/one-wallet . We could consider using https://docs.openzeppelin.com/contracts/2.x/access-control#role-based-access-control , or just to keep it simple and expand operator into an array

[polymorpher]

Is this referring to a context variables passed in by Mocha? It conflicts with this internal variable of the function. Also conflicts with the outer variable this (which also conflicts with the outer function's internal variable)

[polymorpher]

const contract of contracts

[polymorpher]

All looks good. Aside from minor things commented above, the token transfer tests are missing some balance checks and event emission checks. I will re-run some tests when you add them

[polymorpher]

I have these test failures:

  1) AssetManager
       checkTransfer
         Positive transfer test:

      AssertionError: Expected "0" to be equal 1000000000000000000
      + expected - actual

       {
      -  "_hex": "0x0de0b6b3a7640000"
      +  "_hex": "0x00"
         "_isBigNumber": true
       }
      
      at Context.<anonymous> (test/AssetManager.ts:265:12)

  2) AssetManager
       checkAssetManager
         Positive walk-through, deposit, withdraw, approve, send:

      AssertionError: Expected "0" to be equal 1000000000000000000
      + expected - actual

       {
      -  "_hex": "0x0de0b6b3a7640000"
      +  "_hex": "0x00"
         "_isBigNumber": true
       }
      

After they are fixed I can merge the PR

[polymorpher]

I will also make a note here that later we should probably also remove yarn.lock from all folders in the repository per discussion here yarnpkg/yarn#1583 . I am also concerned that specific versions pointed in yarn.lock may unknowingly carry malware (as recent large scale exploits and hacks have shown). So far packages we used and installed seem to be safe. I checked each package and their dependencies one by one

[johnwhitton]

Additional note
After merging changes need to run yarn clean to avoid errors below
Moving forward we should look at whether to include build related files such and deploy related files such as the .openzepplin folder currently we are additionally logging contract addresses in the deploy scripts.

git pull
yarn test
yarn run v1.22.19
warning ../../../package.json: No license field
$ npx hardhat test
Generating typings for: 36 artifacts in dir: typechain for target: ethers-v5
Successfully generated 57 typings!
Compiled 33 Solidity files successfully
An unexpected error occurred:

[Error: ENOENT: no such file or directory, open '/Users/john/one-wallet/sms-wallet/chaincode/build/build-info/a99f4ae7a21a1354b2beba87da171827.json'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/Users/john/one-wallet/sms-wallet/chaincode/build/build-info/a99f4ae7a21a1354b2beba87da171827.json'
}
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

[johnwhitton]

When running
yarn coverage
which uses solidity-coverage
we get

Compilation:
============

Warning: Contract code size exceeds 24576 bytes (a limit introduced in Spurious Dragon). This contract may not be deployable on mainnet. Consider enabling the optimizer (with a low "runs" value!), turning off revert strings, or using libraries.
  --> contracts/AssetManager.sol:28:1:
   |
28 | contract AssetManager is Initializable, PausableUpgradeable, AccessControlEnumerableUpgradeable {
   | ^ (Relevant source part starts here and spans across multiple lines).

Investigating workarounds

1. We already have enabled optimizer in hardhat.config.ts
2. We could change all require statements to if test and revert using errors.
3. We could use diamonds see here and here

I added in hardhat-contract-sizer
And when running yarn compile with optimizer enabled it shows

$ yarn compile
yarn run v1.22.19
warning ../../../package.json: No license field
$ npx hardhat compile
Generating typings for: 36 artifacts in dir: typechain for target: ethers-v5
Successfully generated 57 typings!
Compiled 33 Solidity files successfully
✅ Generated documentation for 36 contracts
 ·-----------------|--------------|----------------·
 |  Contract Name  ·  Size (KiB)  ·  Change (KiB)  │
 ··················|··············|·················
 |  AssetManager   ·       9.131  ·                │
 ·-----------------|--------------|----------------·
✨  Done in 12.54s.

without optimization it shows

 ·-----------------|--------------|----------------·
 |  Contract Name  ·  Size (KiB)  ·  Change (KiB)  │
 ··················|··············|·················
 |  AssetManager   ·      17.063  ·        +7.932  │
 ·-----------------|--------------|----------------·

And yarn deploy with optimizer enabled produces

yarn deploy
yarn run v1.22.19
warning ../../../package.json: No license field
$ npx hardhat deploy
Nothing to compile
No need to generate any newer typings.
✅ Generated documentation for 36 contracts
 ·-----------------|--------------|----------------·
 |  Contract Name  ·  Size (KiB)  ·  Change (KiB)  │
 ··················|··············|·················
 |  AssetManager   ·       9.131  ·                │
 ·-----------------|--------------|----------------·
operators: ["0x70997970C51812dc3A010C7d01b50e0d17dc79C8","0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC","0x90F79bf6EB2c4f870365E785982E1f101E93b906"]
AssetManager deployed to: 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0
AssetManager Operator Threshold: 10
operatorCount : 3
Operator [0]: 0x70997970C51812dc3A010C7d01b50e0d17dc79C8
Operator [1]: 0x3C44CdDdB6a900fa2b585dd299e03d12FA4293BC
Operator [2]: 0x90F79bf6EB2c4f870365E785982E1f101E93b906
AssetManager Global User Auth Limit: 1000000.0
AssetManager Global User Auth Limit: 100000.0
✨  Done in 7.83s.

Will leave optimizer enabled
Believe that this is just an issue with coverage and that when deploying AssetManager will have a size of 9131 bytes which is less than the 24576 bytes limit.

If we have issues with deploy then we can revisit.

[polymorpher]

Is there some flexibilities in the parameters for deploying proxy-upgradeable contracts? Rather than having OpenZepplin to manage everything and hide all the details in obscure local JSON files, ideally we want to keep the address of the proxy, the logic contract's address, and the storage contract's address in some config or environment files, and just load them at times of management / deployment.

Additional note After merging changes need to run yarn clean to avoid errors below Moving forward we should look at whether to include build related files such and deploy related files such as the .openzepplin folder currently we are additionally logging contract addresses in the deploy scripts.

git pull
yarn test
yarn run v1.22.19
warning ../../../package.json: No license field
$ npx hardhat test
Generating typings for: 36 artifacts in dir: typechain for target: ethers-v5
Successfully generated 57 typings!
Compiled 33 Solidity files successfully
An unexpected error occurred:

[Error: ENOENT: no such file or directory, open '/Users/john/one-wallet/sms-wallet/chaincode/build/build-info/a99f4ae7a21a1354b2beba87da171827.json'] {
  errno: -2,
  code: 'ENOENT',
  syscall: 'open',
  path: '/Users/john/one-wallet/sms-wallet/chaincode/build/build-info/a99f4ae7a21a1354b2beba87da171827.json'
}
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

[polymorpher]

Optimizers may get rid of the extra storage slots allocated by the upgradeable contracts (__gap), which are reserved for upgrading logic contracts. I noticed each inherited upgradeable made their own allocation. It is probably not efficient and we may need to allocate our own __gap. Can you check OpenZepplin documentations and developers discussions and see if is some consensus on the recommended way of resolving these issues?