8olidity - claimManager will cause BountyCore::refundDeposit() to fail

[github-actions]

8olidity

high

claimManager will cause BountyCore::refundDeposit() to fail

Summary

Problems arise due to overlapping functionality of the two roles

Vulnerability Detail

The function of BountyCore::refundDeposit() is to implement the refund function, and the refunded assets can be ETH, nft, erc20token. And this function can only be called by depositmanager, and there is also a limit on the calling time.

function refundDeposit(
        bytes32 _depositId,
        address _funder,
      uint256 _volume
) external virtual onlyDepositManager nonReentrant {
    require(!refunded[_depositId], Errors.DEPOSIT_ALREADY_REFUNDED);
    require(funder[_depositId] == _funder, Errors.CALLER_NOT_FUNDER);
    require(
        block.timestamp >= depositTime[_depositId] + expiration[_depositId],
        Errors.PREMATURE_REFUND_REQUEST
    );

    refunded[_depositId] = true;

    if (tokenAddress[_depositId] == address(0)) { //ETH
        _transferProtocolToken(funder[_depositId], _volume);
    } else if (isNFT[_depositId]) {
        _transferNft(
            tokenAddress[_depositId],
            funder[_depositId],
            tokenId[_depositId]
        );
    } else {
        _transferERC20(
            tokenAddress[_depositId],
            funder[_depositId],
            _volume
        );
    }
}

There is also a function claimNft() in the BountyCore contract. This function allows the claimmanager to transfer the nft in the contract away. You can see that this function is the same as refundDeposit(), and the vulnerability also appears in this position.

function claimNft(address _payoutAddress, bytes32 _depositId)
        external
        virtual
        onlyClaimManager
        nonReentrant
    { //@audit  
        _transferNft(
            tokenAddress[_depositId],
            _payoutAddress,
            tokenId[_depositId]
        );
    }

When the claimmanager calls claimNft() to transfer the nft, the DepositManager cannot call refundDeposit() again to redeem the nft. Time limits and caller restrictions are bypassed. The functions of the two roles have conflicting parts, and there is no resolution for this conflict.

poc

describe('claimNft frist', () => {
		it('--------------------------', async () => {
			// ASSUME
			expect(await mockNft.ownerOf(1)).to.equal(owner.address);
	
			// ARRANGE
			const depositId = generateDepositId(Constants.bountyId, 0);
			await atomicContract.connect(depositManager).receiveNft(owner.address, mockNft.address, 1, 1, []);

			// ASSUME
			expect(await mockNft.ownerOf(1)).to.equal(atomicContract.address);
	
			//  
			await atomicContract.connect(claimManager).claimNft(owner.address, depositId);
			
			//  revet
			await atomicContract.connect(depositManager).refundDeposit(depositId, owner.address,0);
			expect(await mockNft.ownerOf(1)).to.equal(owner.address);
	

		});
	});

Impact

claimManager will cause BountyCore::refundDeposit() to fail

Code Snippet

https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L64-L93
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L125-L136

Tool used

Manual Review

Recommendation

Consider this kind of conflict, such as when the bounty is turned off, it is forbidden to call refundDeposit()

Duplicate of #263

[FlacoJones]

Yeah refunded NFT deposits cause many of these exploits. will fix by removing nft deposits for now

[FlacoJones]

OpenQDev/OpenQ-Contracts#114

[FlacoJones]

OpenQDev/OpenQ-Contracts#113