Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add back WhitelistedCrowdsale #1525

Merged
merged 15 commits into from Dec 12, 2018
Merged

Conversation

@nventuro
Copy link
Member

nventuro commented Nov 28, 2018

This creates a new WhitelistedCrowdsale, similar to the pre-2.0 one, but now using Roles.

The creator of the crowdsale is assigned the Whitelister role. This role can be added to other accounts, and cannot be removed (by default).

Whitelisters can also give the Whitelistee role to any acccount. Whitelistees can renounce their role, but cannot give it to other accounts, nor can they remove it from any account.

The crowdsale only processes token purchases where the recipient of the tokens has the Whitelistee role. Note that this doesn't prevent the token to then be transferred to non-Whitelistee accounts: such a restriction would have to be imposed at the token level.

As an experiment, I enhanced the PublicRole.behavior to support the concept of 'managed roles': how each role is tested differs slightly depending on whether the role is managed (Whitelistee) or not (all other roles). This may be extended down the line, as more differences between roles arise, but it suffices for now - I didn't want to go overboard with a full-fledged role configuration object.

Closes #1292

@nventuro nventuro added this to the v2.1 milestone Nov 28, 2018
@nventuro nventuro requested a review from frangio Nov 28, 2018
@mjdietzx

This comment has been minimized.

Copy link

mjdietzx commented Nov 29, 2018

Hey @nventuro, this looks great! The whitelist looks good and the crowdsale is a nice example of using it.

Note that this doesn't prevent the token to then be transferred to non-Whitelistee accounts: such a restriction would have to be imposed at the token level.

From what I've seen, the important thing with security tokens is that at any point in time, accounts holding these tokens can be identified. So I think most use-cases will need to do something like this at the token level as you suggest (performing this check in transfer and transferFrom).

@nventuro

This comment has been minimized.

Copy link
Member Author

nventuro commented Nov 29, 2018

Thanks for reviewing @mjdietzx! One more tiny question, so as to not derail the discussion here: is it token holder identification that's needed, or restriction when transferring? Detection should be doable via events, with no modifications to the contract code.

@mjdietzx

This comment has been minimized.

Copy link

mjdietzx commented Nov 29, 2018

One more tiny question, so as to not derail the discussion here: is it token holder identification that's needed, or restriction when transferring?

At least in our case (regulated in Switzerland / Lichtenstein), we need to be able to identify any token holders. This means given a wallet address, we can tie this to a person's identity. So because we restrict transfers, we can guarantee that only 'known' addresses hold tokens, and therefore we guarantee we can identify them.

If transfers aren't restricted, a known address could purchase tokens during the crowdsale (all good so far), and later transfer them to an 'unknown' address (by any means, i.e. an exchange, p2p). And that would be a problem for us.

@nventuro nventuro self-assigned this Dec 10, 2018
contracts/access/roles/WhitelisteeRole.sol Outdated Show resolved Hide resolved
contracts/mocks/WhitelisterRoleMock.sol Show resolved Hide resolved
contracts/access/roles/WhitelisteeRole.sol Outdated Show resolved Hide resolved
test/access/roles/PublicRole.behavior.js Outdated Show resolved Hide resolved
test/access/roles/PublicRole.behavior.js Outdated Show resolved Hide resolved
test/access/roles/PublicRole.behavior.js Outdated Show resolved Hide resolved
test/crowdsale/WhitelistedCrowdsale.test.js Outdated Show resolved Hide resolved
test/crowdsale/WhitelistedCrowdsale.test.js Outdated Show resolved Hide resolved
test/crowdsale/WhitelistedCrowdsale.test.js Outdated Show resolved Hide resolved
test/crowdsale/WhitelistedCrowdsale.test.js Outdated Show resolved Hide resolved
frangio and others added 8 commits Dec 11, 2018
Co-Authored-By: nventuro <nicolas.venturo@gmail.com>
Co-Authored-By: nventuro <nicolas.venturo@gmail.com>
@nventuro nventuro requested a review from frangio Dec 12, 2018
@nventuro nventuro merged commit 7142e25 into OpenZeppelin:master Dec 12, 2018
2 checks passed
2 checks passed
Travis CI - Pull Request Build Passed
Details
coverage/coveralls First build on whitelisted-crowdsale at 100.0%
Details
@nventuro nventuro deleted the nventuro:whitelisted-crowdsale branch Dec 12, 2018
@Levino

This comment has been minimized.

Copy link

Levino commented Dec 18, 2018

I think there should be some documentation as a markdown file on the functionality of this crowdsale.

@nventuro

This comment has been minimized.

Copy link
Member Author

nventuro commented Dec 18, 2018

@Levino what sort of documentation would you like to see there?

@Levino

This comment has been minimized.

Copy link

Levino commented Jan 3, 2019

Hm.
token.addWhitelister('123'), token.addWhitelisted('123')
Spot the difference? I think it should be token.addWhitelistAdmin('123') instead of the latter. Too dangerous.

@Levino

This comment has been minimized.

Copy link

Levino commented Jan 3, 2019

@mjdietzx Did you manage to implement the following rule: When tokens are to be transferred, the sender and the recipient must have the role Whitelisted. I am only able to check the whitelist status of the sender (with the current implementation of Whitelisted).

@Levino

This comment has been minimized.

Copy link

Levino commented Jan 3, 2019

I added my own modifier to the contract code:

    // Token.sol
    ...
    modifier checkAccess(address account) {
        require(isWhitelisted(account), "only for whitelisted");
        _;
    }

    function transfer(address _to, uint256 _value)
        public
        checkAccess(_to)
        checkAccess(msg.sender)
        returns (bool)
    {
        return super.transfer(_to, _value);
    }
    function transferFrom(address _from, address _to, uint256 _value)
        public
        checkAccess(_to)
        checkAccess(_from)
        returns (bool)
    {
        return super.transferFrom(_from, _to, _value);
    }
    ...
@Levino

This comment has been minimized.

Copy link

Levino commented Jan 3, 2019

I also have another remark about the current design:

So the whitelisting will be automated, I suppose. Lets say we have some server side code that verifies some user input data and then adds an address as whitelisted. Now some attacker gets access to this server and uses the whitelister private key to add himself to the whitelist. Then they could remove all other whitelisters and as such take over the smart contract, right? It would be nice to have an additional Owner who can reset the whitelister-list.

@Levino

This comment has been minimized.

Copy link

Levino commented Jan 4, 2019

Okay, I see now that noone can remove whitelisters. But still the attacker could do a lot of trouble.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.