search

hellobloom / core

:doughnut: Bloom Protocol smart contracts
93 stars 13 forks source link

Review the Bloom Smart Contracts for bugs #35

Closed dereksilva closed 4 years ago

dereksilva commented 5 years ago

Review the smart contracts and tests in this repository. Identify and propose a fix for a bug. Submissions will be reviewed by the Bloom team and rewards will be issued according to the severity. Here are some examples:

Here is our bug bounty payment rubric/guideline: bloom-dev-bounty-rubric

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 1.007 ETH (113.94 USD @ $113.15/ETH) attached to it as part of the Bloom Protocol fund.

aleph-v commented 5 years ago

Hi, I have taken a quick look and the smart contract TokenEscrowMarketplace.sol uses hashes of signature data to disqualify signatures unfortunately this leaves the contract vulnerable to transaction malleability attacks. The SWC Registry covers this issue here. Long story short, given a signature (r,s,v) of hash the signature (-r mod n, s, v) is also a valid signature of hash and its calculation does not require the private key. This modified signature has a different hash so your current usedSignature map will not prevent the reuse of the signature. All signatures can be used twice in TokenEscrowMarketplace.sol in its current format. Here's a link to a proof of concept contract showing signature malleability in action.

ipatka commented 5 years ago

@pvienhage wow great catch! perhaps we should 'burn' or 'invalidate' signatures by invalidating a combination of sender address and nonce instead of signature.

see relevant code in SigningLogic.sol on a new branch here https://github.com/hellobloom/core/blob/trustlessContracts/contracts/SigningLogic.sol

aleph-v commented 5 years ago

Hi @ipatka, yes the best practice is too use data about the transaction instead of the data in the signature. I have read the code you updated and I think that it fixes the issue with signature malleability. Normally I use stored nonces for each address but the formulation where the user signs the nonce they want to use also works.

ipatka commented 5 years ago

ok great. I'm tweaking a couple things to make sure the new system won't be vulnerable to front-running issues where someone can 'burn' the signature contents before a TX is mined. but the overall architecture will stay the same as the most recent code you reviewed

andy8052 commented 5 years ago

Hey, if I have a security concern about contract logic, (not necessarily a bug) where should I be raising those types of issues/do those fall under the bug bounty if my concerns are deemed valid?

ipatka commented 5 years ago

@andy8052 yes that would fall under this bounty

ipatka commented 5 years ago

we'll keep funding this issue and paying out bounties for multiple submissions. not just limited to one submission

andy8052 commented 5 years ago

I haven't noticed from my reading but, is there any way for a user to handle a compromised linked address in AccountRegistryLogic.sol if they have lost the private key (ie. their phone was stolen)?

ipatka commented 5 years ago

Previously we had it so any linked address could unlink any other linked address. But I think it’s safer to only let addresses unlink themselves. If you lose a linked key and want to unlink it from your other addresses you would have to individually unlink each address from the lost address then relink the remaining keys back together. This would be tedious if you have many linked addresses but people will likely have at most 2-3. It could also be combined into a single TX by writing another smart contract to submit these link/ unlink signatures in batches

ipatka commented 5 years ago

If you have attestations that reference the lost address you’ll have to redo those attestations. If the key really was compromised, not just lost you should request the attester to revoke the attestations

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 1 week, 6 days ago. Please review their action plans below:

1) pvienhage has started work.

I'm opening this for the transaction malleability bug I pointed out

Learn more on the Gitcoin Issue Details page.

catageek commented 5 years ago

Hi @ipatka Could you please provide a price offer for each bug/vulnerability with their respective severity ? Also fund your work in gitcoin, because for 1ETH I am afraid you will have a 100$ worth review.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 1.007 ETH (106.82 USD @ $106.08/ETH) has been submitted by:

  1. @pvienhage

@dereksilva please take a look at the submitted work:

ipatka commented 5 years ago

@catageek Yes good point. @dereksilva will post our rubric based on severity and impact

dereksilva commented 5 years ago

@pvienhage We are going to pay out a healthy amount of ETH for the bug you found. I'm having trouble adding more funds to the Gitcoin issue, but as soon as I do they will be sent your way.

gitcoinbot commented 5 years ago
⚡️ A *Bug Finder* Kudos has been sent to @pvienhage for this issue from @dereksilva. ⚡️ The sender had the following public comments: > Phenomenal work! More ETH is on the way soon. Nice work @pvienhage! Your Kudos has automatically been sent in the ETH address we have on file.
gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

The funding of 1.007 ETH (118.11 USD @ $117.29/ETH) attached to this issue has been approved & issued to @pvienhage.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 5.0 ETH (586.44 USD @ $117.29/ETH) attached to it as part of the Bloom Protocol fund.

aleph-v commented 5 years ago

Hi @dereksilva, thanks for the kudos and for the bounty fulfillment! Let me know if there is anything else you need me to do.

dereksilva commented 5 years ago

@catageek We have added guidelines to the issue to provide clarity on how much ETH could be paid out, up to 5 ETH for high severity/high impact issues. We started a new Gitcoin issue at https://gitcoin.co/issue/hellobloom/core/35/1870 for this.

@pvienhage You're welcome! I'll be paying out additional ETH in a few minutes to you for the high/high bug you discovered.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Cancelled

The funding of 5.0 ETH (600.67 USD @ $120.13/ETH) attached to this issue has been cancelled by the bounty submitter

gitcoinbot commented 5 years ago

⚡️ A tip worth 4.00000 ETH (480.54 USD @ $120.13/ETH) has been granted to @pvienhage for this issue from @dereksilva. ⚡️

Nice work @pvienhage! Your tip has automatically been deposited in the ETH address we have on file.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This issue now has a funding of 1.0 ETH (120.13 USD @ $120.13/ETH) attached to it as part of the Bloom Protocol fund.

srisankethu commented 5 years ago

  1. This is good advice for https://github.com/hellobloom/core/tree/master/contracts.

  2. Check Point 6 here. tx.orgin usage here.

  3. Check for Integer overflow. One such case is this. It is better to have a SafeMath library like the linked project.

  4. Finish all state changes first to avoid reentrancy attack. Check this. Found many cases.

I am unable to register via Gitcoin right now. Will register soon.

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

The funding of this issue was increased to 3.0 ETH (341.23 USD @ $113.74/ETH) .

srisankethu commented 5 years ago

@dereksilva Can you update me on my submission? I understand you guys are busy.

ipatka commented 5 years ago

@srisankethu Thanks for reviewing the contracts

Issue 1: Can you clarify which contracts would be affected by assuming they are initialized with a zero balance?

Issue 2 & 3: The contract you referenced is not in scope of this bounty. That Metacoin contract is just dead code that we should remove from the repo. Thanks for pointing it out. The scope fo the bounty is the contracts referenced in the readme: https://github.com/hellobloom/core#contract-summaries

Issue 4: Where did you identify code that could be vulnerable to reentrancy attacks? Can you link to a specific example?

srisankethu commented 5 years ago

@ipatka With Issue 1 and Issue 4, I made a general suggestion. I will look into it where it is affecting. Issue 1 affects where contract is created(in the case where attack has pre-created the contract with the same address with a non-zero balance). I was to the point with Issues 2&3, but did not know they were out of scope. It wasn't mentioned that they were out of scope earlier!?

I will deep dive and look into the contracts thoroughly :+1:

aj07 commented 5 years ago

Just wanted to know, IS this bounty still open?

srisankethu commented 5 years ago
  1. Wouldn't it be better to add a require(!isManager(_manager), "Address is currently a manager"); check to see if the owner is adding a new manager who is already a manager? here @ipatka It would avoid calling again and again. EDIT: Noticed it is out of scope for this bounty.
  2. Shouldn't reward payment be after event completion?
  3. Need suitable checks for the arguments like in this case, add a check like require(_initializer != address(0));. Specifically for public functions.
dereksilva commented 5 years ago

@aj07 Yes, it's still open!

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work for 3.0 ETH (336.16 USD @ $112.05/ETH) has been submitted by:

  1. @srisankethu

@dereksilva please take a look at the submitted work:

ipatka commented 5 years ago

@srisankethu

  1. Yes that protection could be useful but as an admin utility it is very low impact.
  2. What would be the benefit of switching the order?
  3. This is also an admin utility and the initializer is set on deployment. I agree it shouldn't be set to 0 but I don't think we need on chain protection against it.
srisankethu commented 5 years ago

@ipatka

  1. Ok. But it's an improvement right!?
  2. May be in case of DDoS attack when the requests can be come prior to the completion of earlier request, they is a chance for misbehaviour!?
  3. https://github.com/ConsenSys/smart-contract-best-practices/issues/61
ipatka commented 5 years ago

  1. No because that check can be done off chain prior to sending the transaction. On chain input validation should only prevent malicious activity or undesirable state changes.

  2. Are you suggesting there is a reentrancy vulnerability due to emitting an event after a token transfer?

  3. I don’t see how the link you posted is relevant. What does the eth balance have to do with this contract’s logic?

srisankethu commented 5 years ago
  1. I understand. Thank for the info.
  2. Yes, possibility.
  3. Yes the link doesn't make sense, now that I think it about it. IYour explanation to point 1 answers like example suggestion.
srisankethu commented 5 years ago

@ipatka can you update me on my comment. Thank you!

ipatka commented 5 years ago

@srisankethu These are not bugs or vulnerabilities so we will not grant an award for this submission. Thank you for your detailed review! The bounty is still open so feel free to continue pointing out any potential issues and I would be happy to discuss them here

srisankethu commented 5 years ago

@ipatka Thanks for the update. I will 'Stop work' on gitcoin.

gitcoinbot commented 5 years ago

⚡️ A tip worth 0.10000 ETH (8.44 USD @ $84.39/ETH) has been granted to @vikaskyadav for this issue from @dereksilva. ⚡️

Nice work @vikaskyadav! Your tip has automatically been deposited in the ETH address we have on file.

dev1644 commented 5 years ago

Is Bounty still open?

ipatka commented 5 years ago

Yes we’re still accepting submissions for this bounty

dev1644 commented 5 years ago

MiniMeToken.sol and MiniMeVestedToken.sol are of version Version ^0.4.6 & Version ^0.4.15, so the structure is different from other contracts, they don't have Visibility and Getters defined and many more versioning difference, is this okay?

ipatka commented 5 years ago

Yes those contracts are only used by the BLT token so we are not actively updating that code. They are not in the scope of this bounty. The scope fo the bounty is the contracts referenced in the readme: https://github.com/hellobloom/core#contract-summaries

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

Work has been started.

These users each claimed they can complete the work by 1 month, 2 weeks ago. Please review their action plans below:

1) markusj1201 has started work.

  1. Research issues.
  2. Audit contracts
  3. Review code & syntax.
  4. Prototype functions.
  5. Compile code
  6. Implement test
  7. Collect & review errors
  8. Debug
  9. Submit updated contract.

Learn more on the Gitcoin Issue Details page.

gorlitzer commented 5 years ago

hey, is this bounty still open?

dereksilva commented 5 years ago

@gorlitzer Yes, it is!

gitcoinbot commented 5 years ago

Issue Status: 1. Open 2. Started 3. Submitted 4. Done

This Bounty has been completed.

Additional Tips for this Bounty: