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

Conversation

nventuro
Copy link
Contributor

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 feature New contracts, functions, or helpers. contracts Smart contract code. labels Nov 28, 2018
@nventuro nventuro added this to the v2.1 milestone Nov 28, 2018
@nventuro nventuro requested a review from frangio November 28, 2018 16:42
@mjdietzx
Copy link

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
Copy link
Contributor Author

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
Copy link

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
@nventuro nventuro requested a review from frangio December 12, 2018 15:08
@nventuro nventuro merged commit 7142e25 into OpenZeppelin:master Dec 12, 2018
@nventuro nventuro deleted the whitelisted-crowdsale branch December 12, 2018 17:46
@levino
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
Copy link
Contributor Author

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

@levino
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
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
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
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
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
Labels
contracts Smart contract code. feature New contracts, functions, or helpers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants