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

Re-implement parameterizer #28

Merged
merged 15 commits into from Oct 6, 2017

Conversation

@skmgoldin
Copy link
Contributor

commented Oct 3, 2017

Parameterization is considered an unsolved problem in token-curated registries; there is no formal purity in any of the proposed solutions, and there are known attacks. The formal weakness in TCR parameterizers is that they may be put into “stuck states”, EG, the COMMIT_STAGE_LEN parameter could be set to be very long, effectively breaking challenges. The AdChain parameterizer is an attempt at a practically safe if formally-impure cryptosystem which retains the registry's property of being completely decentralized. In this design, financially-motivated token holders are incentivized to be diligent and not allow “breaking” proposals to be accepted. There is no safety net after that, however.

Mechanically, the AdChain parameterizer implements much of the same logic as the registry itself. The main differences are that instead of Listings we use ParamProposals, and deposits are simply returned if a proposal is accepted without a challenge. This is to say, set parameters are not “challenged” the way listings are. Instead, a new proposal is made and either accepted without challenge or, if challenged, perhaps accepted or perhaps rejected. When a new proposal is made, AdToken is at stake and challenges are thereby incentivized.

The AdChain parameterizer has its own parameters separate from those of the registry itself. The parameters prepended with p such as pMinDeposit are parameters of the parameterizer itself. This is a safety feature: because re-parameterizations are potentially dangerous operations, logically all of the working parameters should be set significantly higher than in the registry itself to mitigate spam and un-serious actors from putting the system at risk.

ParamProposals have a processBy date hardcoded to seven days plus the duration of the apply, reveal and commit stages. This is another safety feature which basically requires that ParamProposals be processed within some reasonable period of time after which they cannot be set no matter what (though challenges on them can be resolved for the purposes of disbursing tokens). The reason for this is to prevent an attacker from securing an acceptance vote on a re-parameterization which makes sense at time a, but which the attacker sits on until time b to deploy when that re-parameterization may not make sense. This is implemented here.

Additional safety features include disallowing duplicate re-parameterizations, meaning that if param x is set to a, a proposal to set x to a will be automatically rejected. Furthermore, if a proposal exists to set x to b, follow-on proposals to do the same will be automatically rejected until the first is processed. This is just to reduce spamming, as well as cognitive overhead for voters (“but I thought we rejected that proposal?”).

Not absolutely everything discussed in this PR is actually fully implemented yet, but I would estimate 80% of it is. The test suite is not complete either and it has not been user tested, so it probably has bugs. Furthermore, there is logic related to challenges which is duplicated between Registry.sol and Parameterizer.sol. All of these will be addressed in future pull requests, but I want to merge this to master so we can ship it to the application team by middle of the week. The existing parameterizer is unused by them, so it won’t break anything.

@skmgoldin skmgoldin force-pushed the parameterizer branch 4 times, most recently from 0de7e89 to fa26ea8 Oct 3, 2017

skmgoldin and others added some commits Sep 28, 2017

@skmgoldin skmgoldin force-pushed the parameterizer branch from fa26ea8 to 1807496 Oct 3, 2017

@skmgoldin skmgoldin requested a review from kangarang Oct 3, 2017

@skmgoldin skmgoldin force-pushed the parameterizer branch from 1807496 to 90236bd Oct 3, 2017

@kangarang
Copy link
Contributor

left a comment

Looks good to me @skmgoldin . Will write some tests for this bad boy soon. If you'd feel more comfortable, we can do a code review together, but I've looked over this and I'm comfortable with pulling it for you!

function canBeSet(bytes32 _propID) constant public returns (bool) {
ParamProposal memory prop = proposalMap[_propID];

return (propExists(_propID) && now > prop.appExpiry && now < prop.processBy &&

This comment has been minimized.

Copy link
@kangarang

kangarang Oct 4, 2017

Contributor

Let's test this, but I think propExists(_propID) is extra because now < prop.processBy will effectively take care of this check.

set(prop.name, prop.value);
} else if (challengeCanBeResolved(_propID)) {
resolveChallenge(_propID);
} else if (now > prop.processBy) {

This comment has been minimized.

Copy link
@kangarang

kangarang Oct 4, 2017

Contributor

Safety checks and require(token.transfer(prop.owner, reward));

ParamProposal memory prop = proposalMap[_propID];
uint deposit = get("pMinDeposit");

require(propExists(_propID) && prop.challengeID == 0);

This comment has been minimized.

Copy link
@jamesyoung

jamesyoung Oct 4, 2017

can this require be encapsulated as a modifier?

}

// set flag on challenge being processed
challengeMap[prop.challengeID].resolved = true;

This comment has been minimized.

Copy link
@jamesyoung

jamesyoung Oct 4, 2017

as a design pattern, should the state transition be done before the transfer (even though re-entrancy may not possible here).

@param _challengeID The challengeID to determine a reward for
*/
function determineReward(uint _challengeID) public constant returns (uint) {
require(!challengeMap[_challengeID].resolved && voting.pollEnded(_challengeID));

This comment has been minimized.

Copy link
@kangarang

kangarang Oct 4, 2017

Contributor

Unnecessary check?

@skmgoldin

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2017

Thanks for the code review, will update.

@skmgoldin skmgoldin merged commit f92e9d5 into develop Oct 6, 2017

1 check passed

continuous-integration/codeship Build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.