At present, token holders who propose a listing which is added to a TCR can pull this listing arbitrarily. This conflicts with the protocol which specifies that quasifinalized elements can't be removed from the structure. This needs to be fixed to match the protocol spec.

Currently a target for 0.2.0 release.

Checks such as isWhiteListed should be moved to a modifier for clarity.

post audit contract changes.

Registry Contract changes (Challenge)

Here are some proposed changes to the Registry Contract to try to create consistency between function return values, triggered event values, and the interfaces/structs defined in Computable/Reputable.

Registry Contract changes (Challenge)

Challenge interface [Computable.js]

export interface Challenge {
  rewardPool:Nos; // (remaining) Pool of tokens to be distributed to winning voters
  address:string; // Owner of the challenge
  resolved:boolean; // Indication of whether the challenge has been resolved
  stake:Nos; // Number of tokens at stake for either party during a challenge
  totalTokens:Nos; // (remaining) Number of tokens used in voting by the winning side
  tokenClaims?:TokenClaims; // Indication of whether a voter has claimed a reward yet
}

• Rename address to challenger for clarity and consistency with the contract struct
• Rename tokenClaims to rewardsClaimed to better describe its purpose
• Add commitExpiry
• Add revealExpiry
• Add listingHash to track the associated listing

Challenge struct [Computable Contract]

struct Challenge {
  uint rewardPool;        // (remaining) Pool of tokens to be distributed to winning voters
  address challenger;     // Owner of Challenge
  bool resolved;          // Indication of if challenge is resolved
  uint stake;             // Number of tokens at stake for either party during challenge
  uint totalTokens;       // (remaining) Number of tokens used in voting by the winning side
  mapping(address => bool) tokenClaims; // Indicates whether a voter has claimed a reward yet
}

• Rename tokenClaims to rewardsClaimed to better describe its purpose
• Add commitExpiry
• Add revealExpiry
• Add listingHash to track the associated listing

function challenge() [Computable Contract]

pollID

• Rename pollID to challengeID for consistency

emit Challenge [Computable Contract]

emit _Challenge(
  _listingHash,
  pollID,
  _data,
  commitEndDate,
  revealEndDate,
  msg.sender
);

• Rename pollID to challengeID for consistency
• _data appears to not be used - remove it?
• Rename commitEndDate to commitExpiry for consistency with Listing applicationExpiry
• Rename revealEndDate to revealExpiry for consistency with Listing applicationExpiry
• Add listingHash to track the associated listing

emit ChallengeFailed [Computable Contract]

emit _ChallengeFailed(
  _listingHash,
  challengeID,
  challenges[challengeID].rewardPool,
  challenges[challengeID].totalTokens
);

emit ChallengeSucceeded [Computable Contract]

emit _ChallengeSucceeded(
  _listingHash,
  challengeID,
  challenges[challengeID].rewardPool,
  challenges[challengeID].totalTokens
);

Installing on a fresh environment, and running into some errors:

rbharath@rbharath-XPS-13-9360:~/computable$ npm run truffle:compile

> [email protected] truffle:compile /home/rbharath/computable
> truffle compile

Error: Could not find /home/rbharath/computable/contracts/Parameterizer.sol from any sources; imported from /home/rbhar$th/computable/contracts/registry.sol
    at /home/rbharath/computable/node_modules/truffle/build/webpack:/~/truffle-core/~/truffle-resolver/index.js:76:1
    at /home/rbharath/computable/node_modules/truffle/build/webpack:/~/truffle-core/~/truffle-resolver/~/async/internal$onlyOnce.js:12:1
    at next (/home/rbharath/computable/node_modules/truffle/build/webpack:/~/truffle-core/~/truffle-resolver/~/async/wh$lst.js:68:1)
    at /home/rbharath/computable/node_modules/truffle/build/webpack:/~/truffle-core/~/truffle-resolver/index.js:64:1
    at /home/rbharath/computable/node_modules/truffle/build/webpack:/~/truffle-core/~/truffle-resolver/fs.js:85:1
    at /home/rbharath/computable/node_modules/truffle/build/webpack:/~/truffle-core/~/truffle-resolver/~/async/internal$once.js:12:1
    at replenish (/home/rbharath/computable/node_modules/truffle/build/webpack:/~/truffle-core/~/truffle-resolver/~/asy$c/internal/eachOfLimit.js:59:1)
    at iterateeCallback (/home/rbharath/computable/node_modules/truffle/build/webpack:/~/truffle-core/~/truffle-resolve$/~/async/internal/eachOfLimit.js:49:1)
    at /home/rbharath/computable/node_modules/truffle/build/webpack:/~/truffle-core/~/truffle-resolver/~/async/internal$onlyOnce.js:12:1
    at ReadFileContext.callback (/home/rbharath/computable/node_modules/truffle/build/webpack:/~/truffle-core/~/truffle$resolver/fs.js:81:1)

Any silly mistakes I might be running into? Here's my system details:

rbharath@rbharath-XPS-13-9360:~/computable$ lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 16.04 LTS
Release:        16.04
Codename:       xenial
rbharath@rbharath-XPS-13-9360:~/computable$ npm --version
5.6.0

Linter is complaining that we're not using emit to explicitly mark events. Needs to be fixed:

410:6     warning    Use emit statements for triggering events.                                                        emit

Leakage in data markets is going to be a challenging issue to fit. We need some way to track leakers of a dataset who attempt to sell it outside the bounds of the data market. Digital watermarks attached to each dataset seem to be a good method for this, but the challenge is that it shouldn't be feasible for new members of the data market to recover un-watermarked versions of the dataset. Need to devise a scheme for this.

The recent PR #15 just added support for solium and eslint lint checks. These lint checks should be integrated into Travis-CI so that pull requests which fail lint cannot be merged into the repo.

We need to add coverage to track the coverage provided by our test suite.

https://github.com/ethereum/solidity/projects/4

The upcoming Solidity 0.5 version is looking like it will become the industry standard. In particular, it looks to close a number of gotcha's that trip solidity developers and rewrites the backend to add greater security guarantees. We should migrate the Computable contracts to Solidity 0.5.

It's not clear yet what the timeframe for the migration is. Should we hit this in our 0.1 release or in our 0.2 release for example? There's not too much Solidity 0.5 documentation yet.

A major target for the 0.2.0 release will be proper handling of encrypted data stored off chain (IPFS or S3). This will require new members of data market to obtain access to private data. This key distribution scheme needs to be designed and implemented. Some thoughts on this already, will add more as clarity improves.

All contracts use regular addition (+/-*) instead of using the SafeMath equivalents in a number of places. This opens up potential holes. Should swap out to using the safe equivalents explicitly.

It would be nice to add some fuzz testing of the Computable contracts. This will require some work figuring out how to structure these tests, but would be a nice orthogonal complement to our existing unit test suite.

Some directions on how we might structure the tests:

crytic/echidna#60 (comment)

From @Cryptofin:

We suggest defining a Voting interface (that PLCRVoting implements) for extensibility. With a clearly defined interface, the voting contract can easily be swapped out with a different mechanism, if you or a forking developer wishes to do so.

Let's use this issue to track security changes we'll need to make to leading up to the 0.1.0 release.

• Swap to ERC20 OpenZeppelin contracts #36
• Decide whether we are going to upgrade to solidity 0.5 for the 0.1.0 release #35
• Have thorough unit test coverage of contracts in repo. Aim: 80ish percent. #38

Will modify this list as clarity emerges around our needs.

The TCR design uses event logs to record data about what a particular listing represents. An advantage of this approach is that on-chain data storage costs are avoided.

Recording data this way makes it difficult for applications to get data about a listing. It requires them to query logs, which is not optimal.

Since smart contracts can only read current data on-chain, they cannot trustlessly read data from the TCR. Smart contract applications will have to rely on an oracle to query the event logs, extract the IPFS hash (or other pointer to some data hosting location), and read that data.

In many cases, TCRs are presented as solutions to the oracle problem, so requiring an additional oracle makes the TCR superfluous for smart contract applications wanting to use this TCR as an oracle.

Storing some data in the Listing struct may be useful to applications.

H/T @cryptofinlabs

After following the directions in the README, npm run test fails because it can't find EIP20.json. This is due to the the recent move from the EIP20 to ERC20 token in the computable repo.

computablelabs/computable.js#86 fixes this temporary problem but looking forward, might it be better to keep the computable submodule pinned to a separate, special branch within the computable repo which specifically exists to keep in sync with computable.js master and remove the git submodule update --remote line from the README instructions? This might avert this problem in the future. Let me know if I've misunderstood anything.

We are going to pull in all of our contract dependencies and maintain them here

contracts/Parametizer.sol:63: PROCESSBY is hardcoded. Consider allowing it to be set during initialization for extensibility.

H/T @Cryptofin

Registry poisoning: If an attacker can successfully reparametize Parametizer with malicious values, the Registry and Parametizer contracts can be broken such that new entries or re-parameterizations can't be made.

• If parameters are set to MAX_UINT256, methods will fail when attempting to add them. Examples include:
  • applyStageLen
  • pApplyStageLen
  • pCommitStageLen
  • pRevealStageLen
• This requires an attacker to get a malicious entry in, so the severity of this attack depends on how easily an entry is let through.
• A workaround is to require that these parameters are limited by some upper bound such that the addition operations won't revert.

H/T @Cryptofin for pointing out this attack in the audit.

Need to more closely understand Reparameterization operations and how we can prevent them from being abused.

Solium is complaining about the use of now instead of block.timestamp:

contracts/Parameterizer.sol
143:17    warning    Avoid using 'now' (alias to 'block.timestamp').                                                           ``

We should convert these to block.timestamp to remove lint complaints.

Turns out the issue is trickier. The warning is that block.timestamp can be manipulated by miners. Not clear there's a good fix here.

Going to close the issue.

The Parameterizer contract performs percentage calculations using integer arithmetic and a base fraction of 100. This means that rounding errors have the potential to accumulate and create "dust" tokens that can't be used effectively. Although this isn't a direct security hole, it results in inefficiency and messiness. Swapping to use base 10,000 eliminates this, but requires multiple back and forth interconversions.

Currently not labeling as a release blocker.

We have decent coverage of our contracts through the tests in the computable.js tool, but we don't yet have a good way to measure our coverage right now. We should build a coverage reporting tool that tracks our coverage for the 0.1.0 release.

Our goal is 80ish percent coverage by release.

https://github.com/ConsenSys/mythril is a project building a python-based security analysis tool for solidity contracts. Enable mythril testing on our smart contracts and see what we uncover.

The test suite for this repo is hosted in the computable.js repo. If we're developing a new feature in this repo, we currently test with the following flow:

• Push a [WIP] branch to the computable repo containing the modified code
• Modify the .gitmodules file in a checked-out copy of the computable.js repo to add line branch = [WIP] (replace with the name of your branch).
• Run computable.js tests in your local repo.

This process is unwieldy. We should have a simple command for running these tests; ideally npm run tests from the computable repo should perform this process automatically. It's not clear how we can implement this though.

#26 adds a TODO for adding a Factory contract that creates a plain list of TCRs. In order to construct a full fledged marketplace, we will need to create a TCR of TCRs. The easiest way to do this is maintain a TCR whose entries are the addresses of other TCR contracts.

According to the style guide indentation should use four spaces, as is the usual standard for most languages.

I'm just curious if there's any reason this project uses two. Sorry if this is just noise!

The Parametizer contract doesn't support recording information about why a proposal was made. It's often important to have some discussion about why, since some proposals only make sense with context. Currently, Parameterization relies on existing web infrastructure for discussion, with no decentralization.

H/T @cryptofinlabs

In a few places, we use var to type variables instead of explicit types:

https://github.com/computablelabs/computable/blob/master/contracts/Parameterizer.sol#L191

Replace these with explicit types to avoid surprises.

I'm not currently annotating this as a release blocker.

Specify error message for revert() to help with debugging.

For the 0.1.0 Computable launch, we'll need to launch our contracts onto a test network (ropsten, kovan, etc.). We've done a bit of this already, but it would be good to get this into the repo and documented so we have a standard launch process.

Marking this a blocker for 0.1.0 so we set up a standard launch process we can use in future releases.

We don't need truffles custom output as we don't use their js env anyway. Create a simple script to place the abis in the build dir with just solc

_ApplicationWhitelisted is emitted if an already-whitelisted entry gets challenged, and the challenge is overcome. It's not clear if this behavior is desirable or not.

https://github.com/computablelabs/computable/blob/master/contracts/Registry.sol#L456

The semantics of this need to be scrutinized and either changed or accepted as a feature.

Use this as the master ticket for additional events / modifications to existing events needed.

To kick it off:

• CandidateRemoved - add yea, nay, vote_by to event to retroactively grep for details once listed. This would be for listed and challenge details.

change :all to :recommeded in the .soliumrc.json file for "extends"

We should swap out our use of the Consensys EIP20 contracts for the OpenZeppelin ERC20 contracts. The latter contracts have been audited more and are the current community gold standard.

Although truffle supports simple tests written directly in solidity, it's recommend that more complex tests be written directly in Javascript. We should add tests of existing contracts in solidity.

This PR provides some examples of cases where the Truffle community recommends using the js tests instead of solidity for advanced functionality.

trufflesuite/truffle#644

The contracts currently use very old versions of the solidity compiler (0.4.8 and 0.4.11). Solidity has changed significantly since. We need to upgrade to 0.4.24, especially since it moves us closer to the 0.5 solidity release (#35)

PLCRVoting's startPoll function is public and has no access controls, meaning it can get spammed. This may make it harder for dapps to function in some cases, since reading state about all Polls would be polluted by spam.

Our recommendation is to modify PLCRVoting's startPoll to only be accessible by the Parameterizer contract.

This would require pulling PLCRVoting into the Computable repo explicitly instead of pulling down from EthPM as we're doing currently.

H/T @cryptofinlabs

Similar to the cleanup that just occurred in computable.js, clear out the old alpha code and move in some new alpha code. This time, move in the bare minimum contracts to setup a data-market TCR. Likely https://github.com/skmgoldin/tcr

Also omit the large amount of javascript code here that is supporting the js-testing env as that is now located in computable.js.

Include linting an first via solium, and assure that's wired to travis. Aside from that leave the repo as bare-bones as possible as we ramp-up our .sol staffing.

In follow up tickets I think we can look at pulling in some best-of-breed tools for analysis like mythril (or other...)

Now that we have a small contract and unit test in place, we need to activate Travis CI continuous integrations to make sure future development doesn't break existing contracts.

see #25

With vanilla contracts in place for a TCR, we require a higher-order contract that serves as a registry of those registries.

A Factory such as this also serves as a launching point for the registries it lists. Shifting the deployment costs to the registry creator (vs us), but allowing us to more tightly control checks on adherence to protocol.