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

Find Weaknesses in BountyOne's new smart contract #2

Open
thebkr7 opened this issue May 8, 2018 · 2 comments
Open

Find Weaknesses in BountyOne's new smart contract #2

thebkr7 opened this issue May 8, 2018 · 2 comments

Comments

@thebkr7
Copy link
Contributor

thebkr7 commented May 8, 2018

We are offering a 0.07 ETH bounty for the whoever finds the most dangerous bug(s) in BountyOne's new smart contract.

Please post a reply to this issue with your ETH address and the bug(s).

@bipinkh
Copy link

bipinkh commented May 9, 2018

there are numerous bugs in the contract. here are some that needs to be handled:
1.) you should always double check the balance while making any transfer. For example, one of the such errors is in withdrawFee() function where you just wrote
owner.transfer(_amount);
You should compare the atomicity of all such transactions.

2.) where is fallback function ? It's the pretty basic thing and most important security consideration that you missed here. Numbers of issues arises due to this such as:
What if your contract address received some tokens ?
and many more..

3.) The development of this contract is very immature. You better follow Proxy patterns, make separate contract for Eternal Storage, separate contract for logic, so that you can update your logic or bugs in contract without affecting previous users data.

There are more bugs than those mentioned above. I just wrote few of them here.

@jackwzp
Copy link
Contributor

jackwzp commented May 9, 2018

Hi @bipinkh, thanks for the comments. In general, we're looking for very specific bugs that an attacker can exploit with our current contract. So if you find a bug, can you provide the details on how an attacker and exploit it including the various input parameters they will pass into any function calls.

1.) you should always double check the balance while making any transfer. For example, one of the such errors is in withdrawFee() function where you just wrote
owner.transfer(_amount);

We did check the amount in the first require statement. Also this is function is ownerOnly as well, so the owner address is known and the transfer should be atomic. Can you elaborate a bit on what is the exact bug that an attacker can exploit?

2.) where is fallback function ? It's the pretty basic thing and most important security consideration that you missed here. Numbers of issues arises due to this such as:
What if your contract address received some tokens ?

In solidity since version 0.4.0, if there are no fallback function defined, the contract can not receive ether. The contract must explicitly define it and mark it as payable. So I don't think we have an issue of someone accidentally sending some ether to this contract. If you're referring to receiving ERC20 tokens, we are planning on making our contract accept ERC20 tokens for bounty. Even then, if someone decides to transfer some ERC20 token to our contract address, we can not prevent them from doing so since we don't own the ERC20 contract. We can implement the ability to transfer ERC20 tokens out of our contract.

3.) The development of this contract is very immature. You better follow Proxy patterns, make separate contract for Eternal Storage, separate contract for logic, so that you can update your logic or bugs in contract without affecting previous users data.

This is a good suggestion but we're trying to keep things simple at the moment but can definitely use this pattern in the future. We're building the MVP and don't want to over complicate things. So I don't see this as a bug but a design decision with tradeoffs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants