sinh3ck - sinh3ck - Denial Of Service On fundBountyToken() Of Any Non-whitelisted Tokens

[github-actions]

sinh3ck

high

sinh3ck - Denial Of Service On fundBountyToken() Of Any Non-whitelisted Tokens

sinh3ck

high

Summary

Inconsistent behaviour leads to the possibility of preventing non-whitelisted tokens to be funded at all.

Vulnerability Detail

The function depositManager.fundBountyToken() has inconsistent behaviour and allows timing attacks to be introduced. As an example, on a bounty with a openQTokenWhitelist with TOKEN_ADDRESS_LIMIT of 1:

• If a non-whitelisted token is funded first, further whitelisted tokens can still be funded.
• If a whitelisted token is funded first, further non-whitelisted tokens cannot be funded.

This inconsistency in behaviour allows Denial of Service attacks by frontrunning. A malicious actor may frontrun with fundBountyToken() transactions until the TOKEN_ADDRESS_LIMIT is reached. Added to the fact that it is possible to fund the bounty with _tokenAddress == address(0) and msg.value == 0(since no check of zero-value is made when funding protocol tokens - seeBountyCore.receiveFunds()), a malicious actor may deny service on all minted bounties at virtually no cost (except the cost of gas of making fundBountyToken()` transactions).

As an example of the steps of an exploitation scenario:

1. Several bounties are minted by multiple users.
2. Malicious actor immediately funds the bounties by executing fundBountyToken() transactions with _tokenAddress == address(0) and msg.value == 0 until the TOKEN_ADDRESS_LIMIT is reached on all bounties.
3. No further funding of non-whitelisted tokens is possible on any bounty. It is still possible to fund with whitelisted tokens, but then the concept of non-whitelisted tokens ceases to make sense.

The following PoC can be included in test/ClaimManager.test.js:

// ...

describe('fundBountyTokenExploit', () => {
	it('should revert if funded with a non-whitelisted token and bounty is at funded token address capacity', async () => {
		// ARRANGE
		await openQProxy.mintBounty(Constants.bountyId, Constants.organization, atomicBountyInitOperation);

		const bountyAddress = await openQProxy.bountyIdToAddress(Constants.bountyId);

		await blacklistedMockDai.approve(bountyAddress, 10000000);
		await mockLink.approve(bountyAddress, 10000000);

		// set lower capacity for token
		await openQTokenWhitelist.setTokenAddressLimit(1);

		// malicious actor frontruns with whitelisted token
		await depositManager.fundBountyToken(bountyAddress, ethers.constants.AddressZero, 1, 1, Constants.funderUuid);

		// non-whitelisted token funding is reverted
		// ACT + ASSERT
		await expect(depositManager.fundBountyToken(bountyAddress, blacklistedMockDai.address, 10000000, 1, Constants.funderUuid)).to.be.revertedWith('TOO_MANY_TOKEN_ADDRESSES');
	});
});

// ...

Impact

Ultimately, a malicious actor may prevent funding of any non-whitelisted tokens on all bounties.

Code Snippet

DepositManagerV1.sol-L45

DepositManagerV1.sol-L207

BountyCore.sol-L42

Tool used

Manual Review

Recommendation

It is recommended to make the behaviour consistent in terms of the return of the function depositManager.fundBountyToken() (and inherently depositManager.tokenAddressLimitReached()) when dealing with whitelisted and non-whitelisted tokens.

Duplicate of #62