TrungOre - Function BountyCore.receiveFunds forget checking msg.value > 0 when tokenAddress == address(0)

[github-actions]

TrungOre

high

Function BountyCore.receiveFunds forget checking msg.value > 0 when tokenAddress == address(0)

Summary

Attacker can call BountyCore.receiveFunds() with _tokenAddress = address(0), volume > 0 and msg.value = 0 which can incur some issues for the users.

Vulnerability Detail

Function BounttyCore.receiveFunds() have a check whether _volume > 0 to make sure no1 can call this function without contributing any tokens into the bounty. Unfortunately this check will be true with normal ERC20 tokens not with the native one since the implementation doesn't require the msg.value == volume.

function receiveFunds(
        address _funder,
        address _tokenAddress,
        uint256 _volume,
        uint256 _expiration
    )
        external
        payable
        virtual
        onlyDepositManager
        nonReentrant
        returns (bytes32, uint256)
    {
        require(_volume != 0, Errors.ZERO_VOLUME_SENT);
        require(_expiration > 0, Errors.EXPIRATION_NOT_GREATER_THAN_ZERO);
        require(status == OpenQDefinitions.OPEN, Errors.CONTRACT_IS_CLOSED);

        bytes32 depositId = _generateDepositId();

        uint256 volumeReceived;
        if (_tokenAddress == address(0)) {
            volumeReceived = msg.value;
        } else {
            volumeReceived = _receiveERC20(_tokenAddress, _funder, _volume);
        }

        funder[depositId] = _funder;
        tokenAddress[depositId] = _tokenAddress;
        volume[depositId] = volumeReceived;
        depositTime[depositId] = block.timestamp;
        expiration[depositId] = _expiration;
        isNFT[depositId] = false;

        deposits.push(depositId);
        tokenAddresses.add(_tokenAddress);

        return (depositId, volumeReceived);
    }

So users can call this function without paying anything by using the parameter as follows:

• msg.value = 0
• _volume = x > 0
• _tokenAddress = address(0)

With this flaw, attacker can:

1. execute a lot of deposits to make the BountyCore.deposits[] array grow massively. This will make the function BountyCore.getLockedFunds() out-of-gas to call which will incur the DDOS when funder try to call DepositManagerV1.refundDeposit() to claim their deposits back.
   • BountyCore.getLockedFunds() is out-of-gas because it loops through the entire deposits[] array which is very large
   • DepositManagerV1.refundDeposit() is DDOS because it calls to function BountyCore.getLockedFunds() to get the total locked funds.
2. If the openQTokenWhitelist.TOKEN_ADDRESS_LIMIT() == 1, attacker can front-run to take a slot of ETH in BountyCore.tokenAddresses[]. In case the issuer of bounty wants to fund the competitor with non-whitelisted token, (s)he can't do that because when he call DepositManagerV1.fundBountyToken() it will revert since the token addresses limit is reached.

Impact

• User can call refund to take their deposit back
• Issuer can't pay the competitor with their expected tokens

Code Snippet

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

Tool used

Manual review

Recommendation

Modify BountyCore.receiveFunds() as follows:

function receiveFunds(
        address _funder,
        address _tokenAddress,
        uint256 _volume,
        uint256 _expiration
    )
        external
        payable
        virtual
        onlyDepositManager
        nonReentrant
        returns (bytes32, uint256)
    {
        require(_volume != 0, Errors.ZERO_VOLUME_SENT);
        require(_expiration > 0, Errors.EXPIRATION_NOT_GREATER_THAN_ZERO);
        require(status == OpenQDefinitions.OPEN, Errors.CONTRACT_IS_CLOSED);

        bytes32 depositId = _generateDepositId();

        uint256 volumeReceived;
        if (_tokenAddress == address(0)) {
            /// change here 
            require(_volume == msg.value);
            volumeReceived = msg.value;
        } else {
            /// change here 
            require(msg.value == 0);
            volumeReceived = _receiveERC20(_tokenAddress, _funder, _volume);
        }

        funder[depositId] = _funder;
        tokenAddress[depositId] = _tokenAddress;
        volume[depositId] = volumeReceived;
        depositTime[depositId] = block.timestamp;
        expiration[depositId] = _expiration;
        isNFT[depositId] = false;

        deposits.push(depositId);
        tokenAddresses.add(_tokenAddress);

        return (depositId, volumeReceived);
    }

Duplicate of #77

[trungore]

Escalate for 50 USDC
I believe that #87 and this issue are totally different from #288. #288 just pointed out that the Native token can be lost if it was sent along the _tokenAddress when _tokenAddress != address(0). In the mean time, this issue has shown that attacker can totally abuse the protocol by sending 0 native token when _tokenAddress == address(0) which lead to the DOS attack because of out-of-gas. It's seem like a high severity issue as well.

Beside, this issue is duplicate of #87 which is a duplicate of #288. While #288 is rewarded, this issue is marked as Non-Reward.

[sherlock-admin]

Escalate for 50 USDC
I believe that #87 and this issue are totally different from #288. #288 just pointed out that the Native token can be lost if it was sent along the _tokenAddress when _tokenAddress != address(0). In the mean time, this issue has shown that attacker can totally abuse the protocol by sending 0 native token when _tokenAddress == address(0) which lead to the DOS attack because of out-of-gas. It's seem like a high severity issue as well.

Beside, this issue is duplicate of #87 which is a duplicate of #288. While #288 is rewarded, this issue is marked as Non-Reward.

You've created a valid escalation for 50 USDC!

To remove the escalation from consideration: Delete your comment.
To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[hrishibhat]

Escalation accepted

Valid duplicate of #77

[sherlock-admin]

Escalation accepted

Valid duplicate of #77

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.