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

ControlledAccessCrowdsale contract #812

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 109 additions & 0 deletions contracts/crowdsale/validation/ControlledAccessCrowdsale.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
pragma solidity ^0.4.18;

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


/**
* @title ControlledAccess
* @dev The ControlledAccess contract allows functions to be restricted to users
* that possess a signed authorization from the owner of the contract. This signed
* message includes the address to give permission to and the contract address.
* Both addresses are required to prevent reusing the same authorization message
* on different contract with same owner.
*/
contract ControlledAccessCrowdsale is Crowdsale, Ownable {

/**
* @dev Requires msg.sender to have valid access message.
* @param _sig Valid signature from owner
*/
modifier onlyValidAccess(bytes _sig)
{
require(isValidAccessMessage(msg.sender, _sig));
_;
}

/**
* @dev Verifies if message was signed by owner to give access to _add for this contract.
* Assumes Geth signature prefix.
* @param _add Address of agent with access.
* @param _sig Valid signature from owner
* @return Validity of access message for a given address.
*/
function isValidAccessMessage(address _add, bytes _sig)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about _sender or _address instead of _add?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use ECRecovery here?

view public returns (bool)
{
bytes32 r;
bytes32 s;
uint8 v;

//Extract ECDSA signature variables from `sig`
assembly {
r := mload(add(_sig, 32))
s := mload(add(_sig, 64))
v := byte(0, mload(add(_sig, 96)))
}

// Version of signature should be 27 or 28, but 0 and 1 are also possible versions
if (v < 27) {
v += 27;
}

// Verifying if recovered signer is contract owner
bytes32 hash = keccak256(this, _add);
return owner == ecrecover(
keccak256("\x19Ethereum Signed Message:\n32", hash),
v,
r,
s
);
}

/**
* @dev low level token purchase ***DO NOT OVERRIDE***
* @param _beneficiary Address performing the token purchase
* @param _sig Access message to provide with purchase.
*/
function buyTokens(address _beneficiary, bytes _sig) public payable {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

giving this contract extends from Crowdsale we will still have the inherited buyTokens public function which can be called to bypass the signature check, am I right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice that there is no need to override this function, in fact there is comment mentioning no to do that πŸ˜›
I think you just need to override the _preValidatePurchase function to use your modifier with msg.data, am I right?

Copy link
Contributor Author

@PhABC PhABC Apr 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the problem with not overwriting buyTokens(_beneficiary) is that you can't call buyTokens(_beneficiary) with additionnal msg.data. Data will either be ignored or the function will throw for invalid number of arguments (I believe).

What this means is that it would only be possible to buy tokens via the fallback function, where buyer would be msg.sender. With this, msg.data can be the signature and allow the existing fallback and buyTokens functions to be used.

Or, in other words, the following is not possible :

await this.crowdsale.buyTokens(buyer,{ from: delegateBuyer, value: amount, data: signature });

Where, in this case, msg.data is 0xFUNCTION000000...BUYERADDRESS and signature is omitted.

Copy link
Contributor

@shrugs shrugs Apr 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does the evm handle a data field like

0x + buyTokens function sig + _beneficiary addresss + signature data

? will it call buyTokens with _beneficiary like normal and ignore the signature data part? if so, you could, in the pre-validate, pull the signature from msg.data and validate from there?

uint256 weiAmount = msg.value;
_preValidatePurchase(_beneficiary, weiAmount, _sig);

// calculate token amount to be created
uint256 tokens = _getTokenAmount(weiAmount);

// update state
weiRaised = weiRaised.add(weiAmount);

_processPurchase(_beneficiary, tokens);
TokenPurchase(msg.sender, _beneficiary, weiAmount, tokens);

_updatePurchasingState(_beneficiary, weiAmount);

_forwardFunds();
_postValidatePurchase(_beneficiary, weiAmount);
}

/**
* @dev Extend parent behavior requiring beneficiary to have valid access message
* @param _beneficiary Token beneficiary.
* @param _weiAmount Amount of wei contributed.
* @param _sig Valid signature from owner
*/
function _preValidatePurchase(
address _beneficiary,
uint256 _weiAmount,
bytes _sig)
internal onlyValidAccess(_sig)
{
super._preValidatePurchase(_beneficiary, _weiAmount);
}

/**
* @dev fallback function where the msg.data is the signature
*
*/
function () external payable {
buyTokens(msg.sender, msg.data);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

following what I mentioned above, I guess there is no need to override the fallback function

}
19 changes: 19 additions & 0 deletions contracts/mocks/ControlledAccessCrowdsaleImpl.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
pragma solidity ^0.4.18;

import "../token/ERC20/ERC20.sol";
import "../crowdsale/validation/ControlledAccessCrowdsale.sol";


contract ControlledAccessCrowdsaleImpl is ControlledAccessCrowdsale {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems like we might not need the mock, since it doesn't do anything beyond ControlledAccessCrowdsale?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied the logic from WhitelistedCrowdsaleImpl.sol. I believe the idea would be that users would create a contract such as contract MyTokenCrowdsale is ControlledAccessCrowdsale { ... } which would specify the parameters fed in the Crowdsale.sol constructor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what a user would do, for sure, but since this mock doesn't do anything beyond inherit CrontrolledAccessCrowdsale, we can remove it for simplicity. Usually we'd need a mock if we needed to make the constructor payable or something to make testing easier.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you could combine it with a MintableCrowdsale to test the inheritance while we're at it?


function ControlledAccessCrowdsaleImpl (
uint256 _rate,
address _wallet,
ERC20 _token
)
public
Crowdsale(_rate, _wallet, _token)
{
}

}
95 changes: 95 additions & 0 deletions test/crowdsale/ControlledAccessCrowdsale.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
import ether from '../helpers/ether';

const BigNumber = web3.BigNumber;

require('chai')
.use(require('chai-as-promised'))
.should();

const ControlledAccessCrowdsale = artifacts.require('ControlledAccessCrowdsaleImpl');
const SimpleToken = artifacts.require('SimpleToken');

contract('ControlledAccessCrowdsale', function ([owner, wallet, authorized, unauthorized]) {
const rate = 1;
const value = ether(42);
const tokenSupply = new BigNumber('1e22');

describe('message validity', function () {
beforeEach(async function () {
this.token = await SimpleToken.new();
this.crowdsale = await ControlledAccessCrowdsale.new(rate, wallet, this.token.address);
await this.token.transfer(this.crowdsale.address, tokenSupply);
});

it('should accept valid message to authorized user', async function () {
const ACCESS_MESSAGE = this.crowdsale.address.substr(2) + authorized.substr(2);

// Create the signature using account[0] (owner)
const signature = web3.eth.sign(owner, web3.sha3(ACCESS_MESSAGE, { encoding: 'hex' }));

// Recover the signer address from the generated message and signature.
let isValidMessage = await this.crowdsale.isValidAccessMessage(authorized, signature);

isValidMessage.should.equal(true);
});

it('should reject invalid message to unauthorized user', async function () {
const ACCESS_MESSAGE = this.crowdsale.address.substr(2) + authorized.substr(2);

// Create the signature using account[0] (owner)
const signature = web3.eth.sign(owner, web3.sha3(ACCESS_MESSAGE, { encoding: 'hex' }));

// Recover the signer address from the generated message and signature.
let isValidMessage = await this.crowdsale.isValidAccessMessage(unauthorized, signature);

isValidMessage.should.equal(false);
});

it('should reject access message for another contract for authorized user', async function () {
const ACCESS_MESSAGE = this.token.address.substr(2) + authorized.substr(2);

// Create the signature using account[0] (owner)
const signature = web3.eth.sign(owner, web3.sha3(ACCESS_MESSAGE, { encoding: 'hex' }));

// Recover the signer address from the generated message and signature.
let isValidMessage = await this.crowdsale.isValidAccessMessage(authorized, signature);

isValidMessage.should.equal(false);
});

describe('accepting payments', function () {
it('should accept payments from user with valid access message', async function () {
const ACCESS_MESSAGE = this.crowdsale.address.substr(2) + authorized.substr(2);

// Create the signature using account[0] (owner)
const signature = web3.eth.sign(owner, web3.sha3(ACCESS_MESSAGE, { encoding: 'hex' }));

// console.log(signature)

// fallback with signature as msg.data
await this.crowdsale.sendTransaction({ from: authorized, value: value, data: signature }).should.be.fulfilled;

// call buyTokens function with signature
await this.crowdsale.buyTokens(authorized, signature, { value: value, from: authorized }).should.be.fulfilled;
});

it('should reject payments from users with invalid access message', async function () {
const ACCESS_MESSAGE_FOR_1 = this.crowdsale.address.substr(2) + authorized.substr(2);
const INVALID_ACCESS_MESSAGE = this.token.address.substr(2) + authorized.substr(2);

// Create the signature using account[0] (owner)
const sig1 = web3.eth.sign(owner, web3.sha3(ACCESS_MESSAGE_FOR_1, { encoding: 'hex' }));
const sig2 = web3.eth.sign(owner, web3.sha3(INVALID_ACCESS_MESSAGE, { encoding: 'hex' }));

// No signature provided
await this.crowdsale.send(value).should.be.rejected;

// Signature meant for someone else
await this.crowdsale.buyTokens(unauthorized, sig1, { value: value, from: unauthorized }).should.be.rejected;

// Signature meant for another contract
await this.crowdsale.buyTokens(authorized, sig2, { value: value, from: authorized }).should.be.rejected;
});
});
});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we may want to extract the access_message and signature calculations to a function or move them to the beforeEach block

});