chainNue - current refundDeposit design may cause a claimBounty experience bad for winner

[github-actions]

chainNue

medium

current refundDeposit design may cause a claimBounty experience bad for winner

Summary

current refundDeposit design may cause a claimBounty experience bad for winner

Vulnerability Detail

Since OpenQ is a permission-less, anyone can fund a bounty. If someone fundBounty either with ERC20 or NFT token, then he can call refundDeposit if the _expiration is passed.

This expiration is to lock the deposit for a certain time and currently the OpenQ doesn't have any MINIMUM EXPIRATION state variable, so as long as the expiration > 0 it is accepted.

currently, the refundDeposit only check, if:

• it's already refunded
• if caller is not the funder
• if the expiration is passed

File: BountyCore.sol
69:         require(!refunded[_depositId], Errors.DEPOSIT_ALREADY_REFUNDED);
70:         require(funder[_depositId] == _funder, Errors.CALLER_NOT_FUNDER);
71:         require(
72:             block.timestamp >= depositTime[_depositId] + expiration[_depositId],
73:             Errors.PREMATURE_REFUND_REQUEST
74:         );

When a Bounty is filled with a certain amount of reward for the winner, and it's fulfilled the required total reward needed, but turns out the funder did a refundDeposit when the Bounty status is closed (which no-one can fund the bounty again), then the claim / reward will be lack of reward token. The balance of reward token inside the contract will be less than they should have to cover all the winners rewards.

For example, in Tiered bounty type, this issue may lead to some winner (who are late to claim) doesn't get what they supposed to get, because of this refundDeposit (by some unknown person with small expiration time) decrease the balance of token.

We can't blame the issuer, since they didn't do anything wrong, because anyone can do this irresponsible act, which fund a bounty token, and refundDeposit right after the Bounty is closed, which it's not good for protocol's experiences.

Impact

claimBounty might failed for late claimer because of the token is being refunded to depositor.

Code Snippet

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

File: BountyCore.sol
64:     function refundDeposit(
65:         bytes32 _depositId,
66:         address _funder,
67:         uint256 _volume
68:     ) external virtual onlyDepositManager nonReentrant {
69:         require(!refunded[_depositId], Errors.DEPOSIT_ALREADY_REFUNDED);
70:         require(funder[_depositId] == _funder, Errors.CALLER_NOT_FUNDER);
71:         require(
72:             block.timestamp >= depositTime[_depositId] + expiration[_depositId],
73:             Errors.PREMATURE_REFUND_REQUEST
74:         );
75: 
76:         refunded[_depositId] = true;
77: 
78:         if (tokenAddress[_depositId] == address(0)) {
79:             _transferProtocolToken(funder[_depositId], _volume);
80:         } else if (isNFT[_depositId]) {
81:             _transferNft(
82:                 tokenAddress[_depositId],
83:                 funder[_depositId],
84:                 tokenId[_depositId]
85:             );
86:         } else {
87:             _transferERC20(
88:                 tokenAddress[_depositId],
89:                 funder[_depositId],
90:                 _volume
91:             );
92:         }
93:     }

Tool used

Manual Review

Recommendation

To prevent an early refund, make sure the expiration time is set to at least some minimum time, which can guarantee the deposit is not being withdrew before all winners claimed.

Duplicate of #266

[FlacoJones]

In such cases we simply suggest setting an incredibly long expiration date. This is not a 100% trustless protocol. In many areas we assume the issuer is trusted and we opt for flexibility over immutability in many areas.

[hrishibhat]

Agree with the sponsor comment

[chainNue]

Escalate for 21 USDC

My explanation or wording seems not in the attacker point of view, but this issue is about the protocol design about refund which can lead to vulnerability (winner can't claim their reward for fixed tiered, or less reward for percentage tiered). It's not necessary to be a malicious actor to trigger this refund issue.

with different point of view, this refund issue will lead to

• attacker which is one of winner, gain more token by inflate the token balance, and after they claim, they refund, and
• any valid winner can't claim because token balance decreased due to refund (by someone, either attacker, or some unknown depositor)

most currently valid issues is on the 1st point of view, while mine is on the 2nd, which I think it should be also valid.

---

this issue is similar to #266

An adversary can exploit this by making a deposit of 100 for some token with and _expiration of 1 (second). After the competition has been closed they can refund their deposit causing the contract to be short on funds.

Compared to mine, I think it is different in point of view, #266 is the Winner (which is malicious) do the deposit, and mine is someone (which is not the malicious winner) refund thus decrease the pool size resulting other winner can't claim due to lack of reward.

If you read my Vulnerability Detail section, it mention about depositing for short time (which then increase prize pool) before claiming and then claim and refund (since expiration is short). also, mention about

... in Tiered bounty type, this issue may lead to some winner (who are late to claim) doesn't get what they supposed to get, because of this refundDeposit (by some unknown person with small expiration time) decrease the balance of token.

in #218 (which is part of #266 issues):

If refundDeposit is called after TieredPercentageBounty is closed, it may make the contract balance less than fundingTotals, thus preventing the winner from claiming the prize

Another valid issue #458 (part of #266 issue):

Refunding deposit from a tiered percentage bounty after it was frozen will break reward claims

While for the #275 is similar issue because of refund
the #275 recommendation is

All deposits should be locked for some minimum amount of time after a tiered bounty has been closed

this, inline with this (my) issue recommendation.

---

Judge/sponsor comment about:

... suggest setting an incredibly long expiration date

a suggestion won't resolve this issue, in which some depositor might just skip this suggestion and they just want to make it as short as possible whenever they want, later they won't realized when they refund, it will raise the issue.

[sherlock-admin]

Escalate for 21 USDC

My explanation or wording seems not in the attacker point of view, but this issue is about the protocol design about refund which can lead to vulnerability (winner can't claim their reward for fixed tiered, or less reward for percentage tiered). It's not necessary to be a malicious actor to trigger this refund issue.

with different point of view, this refund issue will lead to

• attacker which is one of winner, gain more token by inflate the token balance, and after they claim, they refund, and
• any valid winner can't claim because token balance decreased due to refund (by someone, either attacker, or some unknown depositor)

most currently valid issues is on the 1st point of view, while mine is on the 2nd, which I think it should be also valid.

---

this issue is similar to #266

An adversary can exploit this by making a deposit of 100 for some token with and _expiration of 1 (second). After the competition has been closed they can refund their deposit causing the contract to be short on funds.

Compared to mine, I think it is different in point of view, #266 is the Winner (which is malicious) do the deposit, and mine is someone (which is not the malicious winner) refund thus decrease the pool size resulting other winner can't claim due to lack of reward.

If you read my Vulnerability Detail section, it mention about depositing for short time (which then increase prize pool) before claiming and then claim and refund (since expiration is short). also, mention about

... in Tiered bounty type, this issue may lead to some winner (who are late to claim) doesn't get what they supposed to get, because of this refundDeposit (by some unknown person with small expiration time) decrease the balance of token.

in #218 (which is part of #266 issues):

If refundDeposit is called after TieredPercentageBounty is closed, it may make the contract balance less than fundingTotals, thus preventing the winner from claiming the prize

Another valid issue #458 (part of #266 issue):

Refunding deposit from a tiered percentage bounty after it was frozen will break reward claims

While for the #275 is similar issue because of refund
the #275 recommendation is

All deposits should be locked for some minimum amount of time after a tiered bounty has been closed

this, inline with this (my) issue recommendation.

---

Judge/sponsor comment about:

... suggest setting an incredibly long expiration date

a suggestion won't resolve this issue, in which some depositor might just skip this suggestion and they just want to make it as short as possible whenever they want, later they won't realized when they refund, it will raise the issue.

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

[hrishibhat]

Escalation accepted

Duplicate of. #266

[sherlock-admin]

Escalation accepted

Duplicate of. #266

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.