Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

KingNFT - DoS attack on unbound deposits array #167

Closed
github-actions bot opened this issue Feb 21, 2023 · 0 comments
Closed

KingNFT - DoS attack on unbound deposits array #167

github-actions bot opened this issue Feb 21, 2023 · 0 comments
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label High A valid High severity issue Reward A payout will be made for this issue

Comments

@github-actions
Copy link

github-actions bot commented Feb 21, 2023

KingNFT

medium

DoS attack on unbound deposits array

Summary

The deposits array of BountyStorageCore contract is unbound, along with the for loop read in getLockedFunds() function which is called by refundDeposit(), lead to a realistic DoS attack vector which would prevent any users from refund.

Vulnerability Detail

Let's illustrate why DoS attack is realistic and feasible.
As shown on L54 of BountyStorageCore contract and L34~L36 of receiveFunds() function

    function receiveFunds(
        // ...
    )
        // ...
    {
        require(_volume != 0, Errors.ZERO_VOLUME_SENT);
        require(_expiration > 0, Errors.EXPIRATION_NOT_GREATER_THAN_ZERO);
        require(status == OpenQDefinitions.OPEN, Errors.CONTRACT_IS_CLOSED);

        //...

        deposits.push(depositId);
        //...
    }

There is no limit on the deposits array length.

Next let's look at the for loop in getLockedFunds() function, L341-349 of BountyCore contract

    function getLockedFunds(address _depositId)
        // ...
    {
        uint256 lockedFunds;
        bytes32[] memory depList = this.getDeposits();
        for (uint256 i = 0; i < depList.length; i++) {
            if (
                block.timestamp <
                depositTime[depList[i]] + expiration[depList[i]] && // @audit 2 SLOAD required
                tokenAddress[depList[i]] == _depositId // @audit 1 SLOAD required
            ) {
                lockedFunds += volume[depList[i]]; // @audit 1 SLOAD
            }
        }

        return lockedFunds;
    }

Each time an deposits item is added, there is 4 additional SLOAD instructions overhead. As one SLOAD costs 2100 gas, so each deposits item costs more than 8400 additional gas to call getLockedFunds() function. As the max gas limit of block is 30M, we can calculate the minimum DoS deposits by

minDoSDeposits = 30M / 8400 = 3572

So if there is more than 3572 deposits, then any transactions containing call to getLockedFunds() can't be mined. It looks like a big number, but the contract will be deployed on Polygon chain, the gas cost for executing one deposit is only about $0.02 (reference). As depositing volume of token can be as low as 1 WEI, the total cost for a successful DoS attack is pretty much the total gas cost which is about

costOfDoSAttack = 3572 * $0.02 = $71.44 

We can see the cost is low, so the attack is realistic and feasible.

Impact

The attack would prevent any users from refund

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Storage/BountyStorageCore.sol#L54

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L333-L352

Tool used

Manual Review

Recommendation

Limit the minimum depositing volume.

Duplicate of #77

@github-actions github-actions bot added Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label High A valid High severity issue labels Feb 21, 2023
@sherlock-admin sherlock-admin added the Reward A payout will be made for this issue label Mar 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label High A valid High severity issue Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

1 participant