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

bin2chen - nftDepositLimit attack #131

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

bin2chen - nftDepositLimit attack #131

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 Medium A valid Medium severity issue Reward A payout will be made for this issue

Comments

@github-actions
Copy link

github-actions bot commented Feb 21, 2023

bin2chen

medium

nftDepositLimit attack

Summary

Malicious increase in the number of NFT, resulting in normal funder can not deposit nft.

Vulnerability Detail

For OUT_OF_GAS attacks, when depositing NFT, the total number cannot be greater than nftDepositLimit

At present, nftDepositLimit is 5, and it cannot be modified

    function receiveNft(
        address _sender,
        address _tokenAddress,
        uint256 _tokenId,
        uint256 _expiration,
        bytes calldata
    ) external onlyDepositManager nonReentrant returns (bytes32) {
        require(
            nftDeposits.length < nftDepositLimit,   //@audit <-------nftDepositLimit is 5, and no way to modify 
            Errors.NFT_DEPOSIT_LIMIT_REACHED
        );

And after the nft refund, the quantity will not be reduced by 1

So if a malicious user executes 5 times: deposit NFT (any in the whitelist), refund the NFT, the issuer will no longer be able to deposit normal NFT

For example:

  1. alice deploy bounty
  2. alice posts information to social media to let others know about the event
  3. Malicious user bob call fundBountyNFT() nft = any nft in whitelist
    , _expiration=1 second
  4. The next transaction (after 12 seconds) bob executes refundDeposit
  5. bob repeat 5 times (step 3,4), only need 60 seconds
  6. When the event is about to end, alice wants to deposit NFT, but finds that she cannot deposit because
nftDeposits.length > nftDepositLimit

suggested to skip this quantity limit if msg.sender is bounty issuer.

Impact

normal funder can not deposit nft.

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/AtomicBountyV1.sol#L132-L135

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/OngoingBountyV1.sol#L133

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/TieredBountyCore.sol#L25-L28

Tool used

Manual Review

Recommendation

    function receiveNft(
        address _sender,
        address _tokenAddress,
        uint256 _tokenId,
        uint256 _expiration,
        bytes calldata _data
    ) external override onlyDepositManager nonReentrant returns (bytes32) {
        require(
+           _sender == issuer || 
            nftDeposits.length < nftDepositLimit,
            Errors.NFT_DEPOSIT_LIMIT_REACHED
        );

Duplicate of #262

@github-actions github-actions bot added Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A valid Medium 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 Medium A valid Medium severity issue Reward A payout will be made for this issue
Projects
None yet
Development

No branches or pull requests

1 participant