search

LinkdropHQ / linkdrop-safe-module

🛡 Linkdrop Module for Gnosis Safe Wallet
GNU General Public License v3.0
4 stars 2 forks source link

✨Linkdrop x Gnosis Safe Smart Contract Bug-bounty✨ #5

Open Kisgus opened 5 years ago

Kisgus commented 5 years ago

Scope

Below smart contracts are within the scope of the bug bounty:

Contracts within scope

LinkdropModule.sol (and all contracts it is inherited from) https://github.com/LinkdropProtocol/linkdrop-safe-module/blob/dev/contracts/module/LinkdropModule.sol

Contracts not within scope https://github.com/LinkdropProtocol/linkdrop-safe-module/tree/dev/contracts/imports https://github.com/LinkdropProtocol/linkdrop-safe-module/tree/dev/contracts/mocks

Payout

Minor discovered bugs, making linkdrop behave in an unexpected harmful way, without putting any funds at risk, will be rewarded with 100 DAI.

Critical vulnerability bugs allowing 3rd parties to steal or lock up funds will be rewarded with 500 DAI.

First come first served, only the first to identify a specific minor or critical bug will be entitled to receive the payout.

Responsible Disclosure

Make sure that you do not share your submission public until we have confirmed it to you, or else you will be disqualified. Issues will be credited on a first come — first serve basis. Issues already known to us or issues already submitted by another user will not be eligible for rewards. Issues can be submitted anonymously.

Submit an empty "submission" via Gitcoin and send your "submission" to hi@linkdrop.io

gitcoinbot commented 5 years ago

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

This issue now has a funding of 500.0 DAI (500.0 USD @ $1.0/DAI) attached to it as part of the LinkdropHQ fund.

L-KH commented 5 years ago

Costly loop

    {
        setManager();
        for(uint i = 0; i < _linkdropSigners.length; i++)
            isLinkdropSigner[_linkdropSigners[i]] = true;
    }

Ethereum is a very resource-constrained environment. Prices per computational step are orders of magnitude higher than with centralized providers. Moreover, Ethereum miners impose a limit on the total number of gas consumed in a block. If array.length is large enough, the function exceeds the block gas limit, and transactions calling it will never be confirmed https://solidity.readthedocs.io/en/develop/security-considerations.html#gas-limit-and-loops

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 2 years, 6 months ago. Please review their action plans below:

1) igor-dulger has started work.

Load code and look for vulnerabilities 2) maskerwind has started work.

1.Start by analyzing the contract with my own developed tools at first. 2.Manually check the vulnerabilities of the contract. 3.Submit the result once a bug is found. 3) robertmcforster has started work.

I'm doing mostly just manual review on the contract. Submitted a bug directly to the team and am now formally submitting it here.

Learn more on the Gitcoin Issue Details page.

igor-dulger commented 5 years ago

Do you have business logic description ? Formal verification won't give a lot of use, you can use some tool for this. For me to check if everything ok I need to understand what your app does. I hope it make sense.

amiromayer commented 5 years ago

@L-KH Hey there, thanks for the feedback, but we don't see it as a vulnerability. Usually this array will take one or two addresses only. And the Gnosis Safe team itself is also using a for loop for the setup function.

amiromayer commented 5 years ago

@igor-dulger Sure, please refer to our technical description

RobertMCForster commented 5 years ago

There's a replay attack that enables malicious users to send transactions from the same signers multiple times. This vulnerability arises if the same signers have multiple linkdrop modules. Because address(this) is not included in the data to be signed, the same claim signature can be used on every linkdrop module the signers have control over, thereby sending a single approved transaction to the receiver multiple different times.

This issue has been fixed after initial reports to the team with address(this) being added in the verifyLinkdropSignerSignature functions on both the ERC20 and ERC721 modules.

gitcoinbot commented 5 years ago

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

Work for 500.0 DAI (500.0 USD @ $1.0/DAI) has been submitted by:

  1. @robertmcforster

@Kisgus please take a look at the submitted work:

gitcoinbot commented 5 years ago

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

The funding of 500.0 DAI (500.0 USD @ $1.0/DAI) attached to this issue has been approved & issued to @RobertMCForster.

Roundtree-Larry commented 5 years ago

LinkdropCommon.claimedTo (LinkdropCommon.sol#11) is never initialized. It is used in:

Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#uninitialized-state-variables