Implement security checks in ERC20 party tokens

[einar-io]

This issue depends on the feature of PTs being published by DC, and thus takes the role as admin.

• Make sure only creator (DC) can mint tokens.
• Make the necessary overflow checks (see OpenZeppelin contracts).

[Ulrik-dk]

If the admin role can be set another way, this does not have to depend on the DC publishing PTs.

[Ulrik-dk]

deadline, including tests: August 28th, 23:59.

[Ulrik-dk]

responsibilities: Thor: suggestions/spec/sequence diagram/sequence explanation, impl.: Einar + Ulrik.

[Ulrik-dk]

thor deadline: August 20th

[Ulrik-dk]

This is my understanding:

1. the erc20 party token contract must have an admin
2. the publisher of the contract must be its default admin
3. a function 'changeAdmin' must be available to change the admin, and this can only be done by the admin
4. mint and burn must require that the message-sender is the admin
5. mint and burn must check for overflow using https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/math/SafeMath.sol
6. the javascript tests must transfer the admin right to the Dagger contract (?)

[Sword-Smith]

This is my understanding:

1. the erc20 party token contract must have an admin

2. the publisher of the contract must be its default admin

3. a function 'changeAdmin' must be available to change the admin, and this can only be done by the admin

4. mint and burn must require that the message-sender is the admin

5. mint and burn must check for overflow using https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/math/SafeMath.sol

6. the javascript tests must transfer the admin right to the Dagger contract (?)

cf. 2: the publisher of the (party token) contract becomes the admin since this instruction is included in the constructor of the solidity contract, as you write. cf. 5: mint, burn, transfer, and transferFrom must all check for overflow. We should use the safeMath.sol library from Open Zeppelin for this. cf. 6: Yes. But we like to call the "Dagger contract" the "Derivative Contract" (DC) in this setup.

[Ulrik-dk]

cf. 2: the publisher of the (party token) contract becomes the admin since this instruction is included in the constructor of the solidity contract, as you write. cf. 5: mint, burn, transfer, and transferFrom must all check for overflow. We should use the safeMath.sol library from Open Zeppelin for this.

Would this be correct, then? https://github.com/einar-io/geth_tools/commit/e91dce6c446c899709b5433f61103020fe7d32df

cf. 6: Yes. But we like to call the "Dagger contract" the "Derivative Contract" (DC) in this setup.

Ok. This has not been done yet in the above mentioned commit.

[Sword-Smith]

Would this be correct, then? einar-io/geth_tools@e91dce6

To me it looks pretty correct, but would you mind having a quick look at how Open Zeppelin is doing this?

[Ulrik-dk]

Superfluous checks removed: https://github.com/einar-io/geth_tools/commit/9f0aaf9683f654fb2798d3dc3811e51cfc9083db

[Ulrik-dk]

This still needs testing etc.

[Sword-Smith]

I think Ulrik already specified the isue pretty well. But since I promised to do it, here goes:

1. mint and burn should only be callable by the address which is admin
2. admin is a field in the party token contract of type address
3. The publisher of the party token contract is assigned as admin in the party token contract's constructor.
4. In our current setup, the publisher of the party token contract is us (in Ethereum lingo: Externally Owned Account) as opposed to another contract.
5. So initially we are the admin of the published party tokens.
6. The admin role must be transferred to the derivative contract. For this, we need a method changeAdmin(address newAdmin) which is only callable by the current admin and which changes the admin to newAdmin.
7. The JavaScript integration tests that we have built must call the changeAdmin method to transfer the admin role to the derivative contract.

For an explanation of Externally Owned Account (EOA), see: https://ethereum.stackexchange.com/questions/5828/what-is-an-eoa-account

[einar-io]

Done.

The admin check has been implemented in the PT contracts and the affected tests have been modified to issue a changeAdmin(DC) between approve() and active() method calls.

All tests pass. Commit: https://github.com/einar-io/geth_tools/commit/3f179f9d34b37230407b62720ce0ddc31e0a60f1

[Sword-Smith]

Seems complete although external eyes on the PT token contract would be nice.