Fix/whitelisted crowdsale #981

shrugs · 2018-06-06T23:14:22Z

🚀 Description

found the issue! truffle-contract's send() function is bad software and doesn't accept an options hash like every other method in the library, so the sender of that transaction is always the default account, which is not authorized and therefore fails the whitelist check.

I also took this opportunity to refactor WhitelistedCrowdsale, Whitelist.sol, and RBAC.sol to get them up to date with various best practices since they were made.

📘 I've reviewed the OpenZeppelin Contributor Guidelines
✅ I've added tests where applicable to test my new functionality.
📖 I've made sure that my contracts are well-documented.
🎨 I've run the JS/Solidity linters and fixed any issues (npm run lint:all:fix).

come-maiz · 2018-06-08T04:57:22Z

contracts/crowdsale/validation/WhitelistedCrowdsale.sol

 import "../../ownership/Ownable.sol";


 /**
 * @title WhitelistedCrowdsale
 * @dev Crowdsale in which only whitelisted users can contribute.
+ * @dev (using explicit Ownable inheritance even though Whitelist already inherits)


any reason for this?

I like being explicit about this when a contract uses features of a transitively-inherited one. But in this case WhitelistedCrowdsale itself doesn't use Ownable after this PR so I'd vote for removing the explicit inheritance.

come-maiz · 2018-06-08T05:01:37Z

contracts/crowdsale/validation/WhitelistedCrowdsale.sol

    isWhitelisted(_beneficiary)
+    internal


according to http://solidity.readthedocs.io/en/v0.4.24/style-guide.html:
"The visibility modifier for a function should come before any custom modifiers.".
I've requested a solium rule for this: duaraghav8/Ethlint#216

come-maiz

It's cool that the tests needed just minor modifications for this to pass :)

As usual, I'm full of questions. Sorry about that. And I have a request on the tests, let me know what do you think.

come-maiz · 2018-06-08T05:03:21Z

contracts/mocks/WhitelistedCrowdsaleImpl.sol



-contract WhitelistedCrowdsaleImpl is WhitelistedCrowdsale {
+contract WhitelistedCrowdsaleImpl is Crowdsale, WhitelistedCrowdsale {


Is this needed?

not needed; I added it to be explicit about the inheritance. But I'm not particularly attached to it ¯_(ツ)_/¯

come-maiz · 2018-06-08T05:04:18Z

contracts/ownership/Whitelist.sol

@@ -11,84 +11,88 @@ import "./rbac/RBAC.sol";
 * @dev This simplifies the implementation of "user permissions".
 */
 contract Whitelist is Ownable, RBAC {
-  event WhitelistedAddressAdded(address addr);
-  event WhitelistedAddressRemoved(address addr);


Why to remove these?

The RoleAdded/Removed events cover this use case :)

come-maiz · 2018-06-08T05:04:55Z

contracts/ownership/Whitelist.sol

  string public constant ROLE_WHITELISTED = "whitelist";

  /**
   * @dev Throws if called by any account that's not whitelisted.
+   * @dev kept as `onlyWhitelisted` for backwards compatibility


If this is deprecated, it's nice to mention the name of the modifier that replaces it.

come-maiz · 2018-06-08T05:06:01Z

contracts/ownership/rbac/RBAC.sol


  /**
   * @dev reverts if addr does not have role
-   * @param addr address
-   * @param roleName the name of the role
+   * @param _operator address


come-maiz · 2018-06-08T05:07:47Z

test/ownership/Whitelist.test.js

@@ -27,7 +27,7 @@ contract('Whitelist', function (accounts) {
    it('should add address to the whitelist', async function () {
      await expectEvent.inTransaction(
        mock.addAddressToWhitelist(whitelistedAddress1, { from: owner }),
-        'WhitelistedAddressAdded'
+        'RoleAdded'


Ahh, I see, we were triggering two events.
It would be nice to check here that the role added is whitelist.

come-maiz · 2018-06-08T05:08:03Z

test/ownership/Whitelist.test.js

@@ -47,7 +47,7 @@ contract('Whitelist', function (accounts) {
    it('should remove address from the whitelist', async function () {
      await expectEvent.inTransaction(
        mock.removeAddressFromWhitelist(whitelistedAddress1, { from: owner }),
-        'WhitelistedAddressRemoved'
+        'RoleRemoved'


same comment about checking the role name.

shrugs · 2018-06-08T20:41:02Z

updated!

wilhempujar · 2018-06-09T12:04:15Z

Nice @shrugs and @ElOpio ! This has been an issue on our end as well :)

updated

frangio

Code quality improvements! 👏

Left some comments here and there.

Also, to be clear, this PR is not fixing #979 because that issue was resolved by proper use of Truffle, right?

frangio · 2018-06-10T18:39:41Z

contracts/crowdsale/validation/WhitelistedCrowdsale.sol

 import "../../ownership/Ownable.sol";


 /**
 * @title WhitelistedCrowdsale
 * @dev Crowdsale in which only whitelisted users can contribute.
+ * @dev (using explicit Ownable inheritance even though Whitelist already inherits)


I like being explicit about this when a contract uses features of a transitively-inherited one. But in this case WhitelistedCrowdsale itself doesn't use Ownable after this PR so I'd vote for removing the explicit inheritance.

frangio · 2018-06-10T18:39:56Z

contracts/ownership/rbac/RBAC.sol

-  event RoleAdded(address addr, string roleName);
-  event RoleRemoved(address addr, string roleName);
+  event RoleAdded(address indexed _operator, string _role);
+  event RoleRemoved(address indexed _operator, string _role);


Do we have a convention for underscores in event arguments?

I hate that these are breaking changes but string _role should probably be indexed as well. I bet you didn't make it so because of the issue with indexed strings. This got me thinking and I opened #992 to discuss some workarounds.

I don't think we have a convention, no. Actually now that you mention it, I don't think I've seen underscored event args, so I'll remove that.

frangio · 2018-06-10T18:48:03Z

contracts/mocks/WhitelistMock.sol

@@ -7,6 +7,7 @@ contract WhitelistMock is Whitelist {

  function onlyWhitelistedCanDoThis()
    onlyWhitelisted
+    isWhitelisted(msg.sender)


One of the two modifiers is redundant here right?

Yup! Was being lazy and tested both codepaths at the same time. Honestly I'm just going to remove isWhitelisted and stick wtih onlyWhitelisted(address) because that's better anyway

frangio · 2018-06-10T18:58:24Z

contracts/ownership/Whitelist.sol

+  /**
+   * @dev Throws if beneficiary is not whitelisted.
+   */
+  modifier isWhitelisted(address _beneficiary) {


I'm not sure about this modifier name. I see that it's the same modifier used in WhitelistedCrowdsale, but I'm more used to modifiers written with something like only... or when....

is... doesn't feel like it modifies anything to me. I'd love to hear some thoughts on this, and to figure out a reasonable convention for modifier names that we can document somewhere in the project.

come-maiz

Big +1. Thank you @shrugs.
I like onlyIfWhitelisted. But I'm not very good at naming, I'm only good at complaining about bad names :)

…heritance

* fix: swithc WhitelistedCrowdsale to use Whitelist.sol * feat: refactor whitelist.sol, rbac.sol and whitelistedcrowdsale.sol * feat: add event arg assets and update whitelist * fix: update modifier comment and also test isWhitelisted * fix: remove onlyWhitelisted backwards compat attempt, fix explicit inheritance * fix: remove underscore prefix from event args * fix: user access/Whitelist

shrugs requested a review from come-maiz June 6, 2018 23:14

come-maiz reviewed Jun 8, 2018

View reviewed changes

come-maiz previously requested changes Jun 8, 2018

View reviewed changes

frangio mentioned this pull request Jun 10, 2018

Convention around indexed string arguments in events (RBAC) #992

Closed

frangio reviewed Jun 10, 2018

View reviewed changes

shrugs force-pushed the fix/whitelisted-crowdsale branch from 1cdf818 to 03c42f8 Compare June 10, 2018 22:41

come-maiz approved these changes Jun 15, 2018

View reviewed changes

shrugs added 7 commits June 15, 2018 11:54

fix: swithc WhitelistedCrowdsale to use Whitelist.sol

f5c7948

feat: refactor whitelist.sol, rbac.sol and whitelistedcrowdsale.sol

45eed54

feat: add event arg assets and update whitelist

415c929

fix: update modifier comment and also test isWhitelisted

ce02c06

fix: remove onlyWhitelisted backwards compat attempt, fix explicit in…

eb4fa41

…heritance

fix: remove underscore prefix from event args

3f7c254

fix: user access/Whitelist

c7dbf17

shrugs force-pushed the fix/whitelisted-crowdsale branch from 03c42f8 to c7dbf17 Compare June 15, 2018 19:17

shrugs merged commit 92b695f into OpenZeppelin:master Jun 15, 2018

shrugs deleted the fix/whitelisted-crowdsale branch June 15, 2018 21:11

frangio mentioned this pull request Jul 24, 2018

Linearization of inheritance graph impossible as of v1.11.0 #1110

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/whitelisted crowdsale #981

Fix/whitelisted crowdsale #981

shrugs commented Jun 6, 2018 •

edited

Loading

come-maiz Jun 8, 2018

frangio Jun 10, 2018

come-maiz Jun 8, 2018

come-maiz left a comment

come-maiz Jun 8, 2018

shrugs Jun 8, 2018

come-maiz Jun 8, 2018

shrugs Jun 8, 2018

come-maiz Jun 8, 2018

come-maiz Jun 8, 2018

come-maiz Jun 8, 2018

come-maiz Jun 8, 2018

shrugs commented Jun 8, 2018

wilhempujar commented Jun 9, 2018

frangio left a comment

frangio Jun 10, 2018

frangio Jun 10, 2018

shrugs Jun 10, 2018

frangio Jun 10, 2018

shrugs Jun 10, 2018

frangio Jun 10, 2018

come-maiz left a comment



		contract WhitelistedCrowdsaleImpl is WhitelistedCrowdsale {
		contract WhitelistedCrowdsaleImpl is Crowdsale, WhitelistedCrowdsale {

Fix/whitelisted crowdsale #981

Fix/whitelisted crowdsale #981

Conversation

shrugs commented Jun 6, 2018 • edited Loading

🚀 Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

come-maiz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrugs commented Jun 8, 2018

wilhempujar commented Jun 9, 2018

frangio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

come-maiz left a comment

Choose a reason for hiding this comment

shrugs commented Jun 6, 2018 •

edited

Loading