bearonbike - Claim workflow does not update BountyStorageCore.volume which could lead to refund unworkable.

[github-actions]

bearonbike

high

Claim workflow does not update BountyStorageCore.volume which could lead to refund unworkable.

Summary

In BountyCore.getLockedFunds, BountyStorageCore.volume is accumulated as lockedfunds, and claim function of OngoingBounty does not deduct BountyStorageCore.volume.
If user claim some tokens, the BountyStorageCore.volume may be outdated and BountyCore.getLockedFunds could get wrong.
Since DepositManager.refundDeposit calculate specific token's availableFunds as all token's number minus locked token's number(i.e unexpired token number), then the result of availableFunds could be wrong.
If issuer close the bounty before refund, the funder can't make deposit refundable through extend it's expiration, fund could be stucked in a long time.

Vulnerability Detail

For example:
1, Alice mint an ongoingbounty
2, Bob funds 100 Link, set expiration to 10 days.
3, Dave deposit 200 Link, set expiration to 365 days.
4, 11 days have passed, Bob's deposite expired, should be able to refund.
5, Charlie claim 100 Link.
6, Alice close the bounty.
7, Bob try to refund first deposit, but receive nothing due to wrong value calculated by BountyCore.getLockedFunds.
8, Bob can't extend expiration of first deposit to make it refundable owing to bounty had been closed.
9, Bob can't get his money back due to he had already refund

Below is poc script, add it to DepositManager.test.js

it('Claim workflow does not update BountyStorageCore.volume which could lead to refund unworkable.', async () => {
    [Alice, Bob, Charile, Dave] = await ethers.getSigners();
    // Alice mint bounty.
    await openQProxy.connect(Alice).mintBounty(Constants.bountyId, Constants.organization, ongoingBountyInitOperation);
    const bountyAddress = await openQProxy.bountyIdToAddress(Constants.bountyId);

    await mockLink.connect(owner).transfer(Bob.address, 100);
    await mockLink.connect(Bob).approve(bountyAddress, 100);
    await mockLink.connect(owner).transfer(Dave.address, 200);
    await mockLink.connect(Dave).approve(bountyAddress, 200);
    await mockLink.approve(bountyAddress, 10000000);

    // Bob fund bounty 100 Link, set set expiration to 10 days.
    const linkDepositId = generateDepositId(Constants.bountyId, 0);
    await depositManager.connect(Bob).fundBountyToken(bountyAddress, mockLink.address, 100, 10, Constants.funderUuid);

    // time passed, Bob's deposit has expired, should be able to refund his token.
    const elevenDays = 11;
    ethers.provider.send("evm_increaseTime", [elevenDays]);

    // Dave fund 200 link, set expiration to 365 days.
    await depositManager.connect(Dave).fundBountyToken(bountyAddress, mockLink.address, 200, 365, Constants.funderUuid);

    // Charile claim 100 link.
    let abiEncodedOngoingCloserData = abiCoder.encode(['address', 'string', 'address', 'string'], [owner.address, "FlacoJones", owner.address, "https://github.com/OpenQDev/OpenQ-Frontend/pull/398"]);
    await claimManager.connect(Charile).claimBounty(bountyAddress, oracle.address, abiEncodedOngoingCloserData);
    expect(await mockLink.balanceOf(Charile.address)).to.equal(100);

    // Alice close the bounty.
    openQProxy.connect(Alice).closeOngoing(Constants.bountyId);
    // Before refund, bounty's balance.
    const bountyMockTokenBalance = (await mockLink.balanceOf(bountyAddress)).toString();
    expect(bountyMockTokenBalance).to.equal('200');

    // Before refund, Bob's balance.
    const bobMockTokenBalance = (await mockLink.balanceOf(Bob.address)).toString();
    expect(bobMockTokenBalance).to.equal('0');

    // Refund Bob's deposit, should receive 100 Link
    await depositManager.connect(Bob).refundDeposit(bountyAddress, linkDepositId);
    // But it's not work
    const newBobMockTokenBalance = (await mockLink.balanceOf(Bob.address)).toString();
    expect(newBobMockTokenBalance).to.equal('0');

    // Link stucked in bounty
    const newBountyMockTokenBalance = (await mockLink.balanceOf(bountyAddress)).toString();
    expect(newBountyMockTokenBalance).to.equal('200');

    // And extendDeposit is not unworkable.
    await expect(depositManager.connect(Bob).extendDeposit(bountyAddress, linkDepositId, 2)).to.be.revertedWith('CONTRACT_IS_CLOSED');

    // One year passed, Dave's deposit expired. 
    const threeDays = 365;
    ethers.provider.send("evm_increaseTime", [threeDays]);

    // Bob still can't get his money back, due to he had already refunded.
    await expect(depositManager.connect(Bob).refundDeposit(bountyAddress, linkDepositId)).to.be.revertedWith('DEPOSIT_ALREADY_REFUNDED');
});

Impact

Fund can't be returned.

Code Snippet

BountyCore.getLockedFunds
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L333-L352
DepositManager.refundDeposit
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/DepositManager/Implementations/DepositManagerV1.sol#L152-L195

Tool used

Manual Review

Recommendation

Update BountyStorageCore.volume when doing claim.

Duplicate of #256

[FlacoJones]

OpenQDev/OpenQ-Contracts#117

[IAm0x52]

Dupe of #256