ltyu - ERC721 can be deposits as ERC20

[github-actions]

ltyu

high

ERC721 can be deposits as ERC20

Summary

Vulnerability Detail

According to the natspec documentation in DepositManagerV1.sol, fundBountyToken() should be used to "Transfers protocol token or ERC20 from msg.sender to bounty address". However, it is possible to use this function to deposit ERC721 tokens. This is because there is no validation within Bounty.receiveFunds() that prevents ERC721 tokens from being deposited. This results in ERC721 tokens becoming unrefundable, as the deposit has been erroneously registered as a non-NFT.

Impact

Erronous ERC20 deposits with ERC721 tokens will be reverted when attempting to call refundDeposit()

Code Snippet

Heres the unit test for this:

it('should revert when attempting to refund ERC721-as-ERC20 deposit', async () => {
	// ARRANGE
	await openQProxy.mintBounty(Constants.bountyId, Constants.organization, atomicBountyInitOperation);
	const bountyAddress = await openQProxy.bountyIdToAddress(Constants.bountyId);
	await mockNft.approve(bountyAddress, 1);
	const bounty = await AtomicBountyV1.attach(bountyAddress);
	const depositId = generateDepositId(Constants.bountyId, 0);

	// ASSUME
	expect(await mockNft.ownerOf(1)).to.equal(owner.address);

	// ACT
	await depositManager.fundBountyToken(bountyAddress, mockNft.address, 1, 1, Constants.funderUuid);

	// ASSERT
	expect(await bounty.isNFT(depositId)).to.be.false;
	expect(await mockNft.ownerOf(1)).to.equal(bountyAddress);
	await expect(depositManager.refundDeposit(bountyAddress, depositId)).to.be.reverted;
});

This function can be used to transfer both, ERC20 and ERC721:
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L197-L215
Here's the code being impacted:

Tool used

Manual Review

Recommendation

Add validation to receiveFunds() to prevent tokens besides ERC20 and protocol tokens from being deposited.

Duplicate of #352