Review the Bloom Smart Contracts for bugs

[dereksilva]

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:

• Public/external/internal/private on functions. Check to make sure anything public cannot be abused.
• Proper modifiers to restrict access to certain functions, like onlyDuringInitialization.
• Front-running attacks: can someone submit delegated transactions in a different order or use the same signature in different functions to have different effects?
• Token escrow: can tokens ever be minted? Check the math for adjusting escrow balances.
• Signature re-use: are all signatures burned after using them so they can't be replayed?
• Test coverage: run the tests (./bin/test) and review the test cases; check for missing test scenarios.
• Third-party party contracts like Ownable.sol, SafeMath, SafeERC20. Are they used appropriately?

Here is our bug bounty payment rubric/guideline:

[gitcoinbot]

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.

• If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $35,496.69 more funded OSS Work available on the Gitcoin Issue Explorer

[aleph-v]

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]

@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]

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]

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]

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]

@andy8052 yes that would fall under this bounty

[ipatka]

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

[andy8052]

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]

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]

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]

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]

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]

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:

• PR by @pvienhage

---

• Learn more on the Gitcoin Issue Details page
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $33,381.24 more funded OSS Work available on the Gitcoin Issue Explorer

[ipatka]

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

[dereksilva]

@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]

⚡️ 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]

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.

• Learn more on the Gitcoin Issue Details page
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $34,665.72 more funded OSS Work available on the Gitcoin Issue Explorer

[gitcoinbot]

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.

• If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $34,904.41 more funded OSS Work available on the Gitcoin Issue Explorer

[aleph-v]

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]

@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]

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

• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $34,439.95 more funded OSS Work available on the Gitcoin Issue Explorer

[gitcoinbot]

⚡️ 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.

• $34439.95 in Funded OSS Work Available at: https://gitcoin.co/explorer
• Incentivize contributions to your repo: Send a Tip or Fund a PR
• No Email? Get help on the Gitcoin Slack

[gitcoinbot]

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.

• If you would like to work on this issue you can 'start work' on the Gitcoin Issue Details page.
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $34,560.08 more funded OSS Work available on the Gitcoin Issue Explorer

[srisankethu]

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]

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) .

• If you want to claim the bounty you can do so here
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $34,883.98 more funded OSS Work available on the Gitcoin Issue Explorer

[srisankethu]

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

[ipatka]

@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]

@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 👍

[aj07]

Just wanted to know, IS this bounty still open?

[srisankethu]

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]

@aj07 Yes, it's still open!

[gitcoinbot]

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:

• PR by @srisankethu

---

• Learn more on the Gitcoin Issue Details page
• Want to chip in? Add your own contribution here.
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $35,306.37 more funded OSS Work available on the Gitcoin Issue Explorer

[ipatka]

@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]

@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. Don't assume contracts are created with zero balance Consensys/smart-contract-best-practices#61

[ipatka]

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]

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]

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

[ipatka]

@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]

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

[gitcoinbot]

⚡️ 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.

• $43988.14 in Funded OSS Work Available at: https://gitcoin.co/explorer
• Incentivize contributions to your repo: Send a Tip or Fund a PR
• No Email? Get help on the Gitcoin Slack

[dev1644]

Is Bounty still open?

[ipatka]

Yes we’re still accepting submissions for this bounty

[dev1644]

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]

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]

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]

hey, is this bounty still open?

[dereksilva]

@gorlitzer Yes, it is!

[gitcoinbot]

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

---

This Bounty has been completed.

Additional Tips for this Bounty:

• dereksilva tipped 0.1000 ETH worth 16.43 USD to vikaskyadav.
• dereksilva tipped 4.0000 ETH worth 657.29 USD to pvienhage.

---

• Learn more on the Gitcoin Issue Details page
• Questions? Checkout Gitcoin Help or the Gitcoin Slack
• $58,844.73 more funded OSS Work available on the Gitcoin Issue Explorer