libratus - Locked funds calculation is invalid if part of the reward pool was claimed

[github-actions]

libratus

medium

Locked funds calculation is invalid if part of the reward pool was claimed

Summary

If part of the reward pool is claimed, getLockedFunds calculation in BountyCore will be invalid which may lead to errors when trying to refund a deposit

Vulnerability Detail

In getLockedFunds locked funds are calculated the following way:

        for (uint256 i = 0; i < depList.length; i++) {
            if (
                block.timestamp <
                depositTime[depList[i]] + expiration[depList[i]] &&
                tokenAddress[depList[i]] == _depositId
            ) {
                lockedFunds += volume[depList[i]];
            }
        }

Using volume[depList[i]] means always taking full deposit amount. This works for Atomic bounty but not for other types where it is possible to claim rewards in parts. If rewards were partially claimed, then only the remaining portion of the deposit should be considered locked. Current logic can lead to errors as shown in the PoC:

• Deposit 100 DAI with expiration = 50
• Deposit 50 DAI with expiration = 1
• Rewind 10 seconds forward so that second deposit is expired
• Claim a bounty (70 DAI)
• Attempt to refund second deposit of 50 DAI
• Despite the fact that contract has enough DAI, this will revert due to the error in locked funds calculation

diff --git a/test/DepositManager.test.js b/test/DepositManager.test.js
index 00338d3..3bccf4c 100755
--- a/test/DepositManager.test.js
+++ b/test/DepositManager.test.js
@@ -545,6 +545,42 @@ describe('DepositManager.sol', () => {
                                expect(newFunderFakeTokenBalance).to.equal('10000000000000000000000');
                        });
 
+                       it('should transfer balance to funder after partial claim', async () => {
+                               // ARRANGE
+                               await openQProxy.mintBounty(Constants.bountyId, Constants.organization, ongoingBountyInitOperation);
+
+                               const bountyAddress = await openQProxy.bountyIdToAddress(Constants.bountyId);
+                               const bounty = await OngoingBountyV1.attach(bountyAddress);
+
+                               await openQProxy.setPayout(Constants.bountyId, mockDai.address, 70);
+
+                               await mockDai.approve(bountyAddress, 10000000);
+
+                               const daiDepositId = generateDepositId(Constants.bountyId, 0);
+                               await depositManager.fundBountyToken(bountyAddress, mockDai.address, 100, 50, Constants.funderUuid);
+
+                               const daiDeposit2Id = generateDepositId(Constants.bountyId, 1);
+                               await depositManager.fundBountyToken(bountyAddress, mockDai.address, 50, 1, Constants.funderUuid);
+
+                               // ASSUME
+                               const bountyDaiBalance = (await mockDai.balanceOf(bountyAddress)).toString();
+                               expect(bountyDaiBalance).to.equal('150');
+
+                               // 10 ticks forward. Second deposit expired, first is still active.
+                               ethers.provider.send("evm_increaseTime", [10]);
+
+                               // Claim 70 tokens. 80 remain on the contract
+                               await claimManager.connect(oracle).claimBounty(bountyAddress, owner.address, abiEncodedOngoingCloserData);
+
+                               // ACT
+                               // Try refund 50 tokens. 80 remain on the contract so this shouldn't fail but it does
+                               await depositManager.refundDeposit(bountyAddress, daiDeposit2Id);
+
+                               // // ASSERT
+                               const newBountyFakeTokenBalance = (await mockDai.balanceOf(bountyAddress)).toString();
+                               expect(newBountyFakeTokenBalance).to.equal('30');
+                       });
+
                        it('should transfer NFT from bounty contract to funder', async () => {
                                // ASSUME
                                expect(await mockNft.ownerOf(1)).to.equal(owner.address);

Impact

Under certain circumstances depositor will not be able to refund an expired deposit

Code Snippet

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

Tool used

Manual Review

Recommendation

Subtract the amount of claimed tokens when calculating locked funds. Amount of claimed tokens can be tracked separately

Duplicate of #257

[FlacoJones]

Will fix by simply removing crowdfunding and restricting funding to the bounty issuer

[FlacoJones]

OpenQDev/OpenQ-Contracts#117 and OpenQDev/OpenQ-Contracts#112 and OpenQDev/OpenQ-Contracts#116

[IAm0x52]

Dupe of #257

[kiseln]

Escalate for 20 USDC

I don't think it's a duplicate of 257. Even though both likely originate from the same design choices, the issues are different in impact and repro steps.

257 advocates for proportional distribution of remaining refunds to depositors. It describes a scenario when all refunds are expired and ready to be refunded.

The current issue, on the other hand, highlights incorrect calculations when determining the amount that can be refunded. In order to reproduce it, you need to have some deposits expired and some still ongoing. The impact is also about tokens being locked in the contract unnecessarily until the other deposit expires.

In my opinion, there is a distinction and it is possible to fix one but not the other and vice versa.

#421 is the same as this one, btw.

[sherlock-admin]

Escalate for 20 USDC

I don't think it's a duplicate of 257. Even though both likely originate from the same design choices, the issues are different in impact and repro steps.

257 advocates for proportional distribution of remaining refunds to depositors. It describes a scenario when all refunds are expired and ready to be refunded.

The current issue, on the other hand, highlights incorrect calculations when determining the amount that can be refunded. In order to reproduce it, you need to have some deposits expired and some still ongoing. The impact is also about tokens being locked in the contract unnecessarily until the other deposit expires.

In my opinion, there is a distinction and it is possible to fix one but not the other and vice versa.

#421 is the same as this one, btw.

You've created a valid escalation for 20 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.

[Evert0x]

Escalation rejected.

It's a duplicate of #257 as it originates from the same design choice. 257 might have a different scenario and recommendation but it's based on the same flaw in the code.

[sherlock-admin]

Escalation rejected.

It's a duplicate of #257 as it originates from the same design choice. 257 might have a different scenario and recommendation but it's based on the same flaw in the code.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.