Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

ltyu - Partial refunds can cause the rest of the deposit to be lost #259

Closed
github-actions bot opened this issue Feb 21, 2023 · 3 comments
Closed
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed

Comments

@github-actions
Copy link

github-actions bot commented Feb 21, 2023

ltyu

high

Partial refunds can cause the rest of the deposit to be lost

Summary

Partial funds that are refunded will cause the rest of the funds to be lost.

Vulnerability Detail

In refundDeposit() of DepositManagerV1.sol, deposit refunds are calculated using the unlocked/available funds. If the deposit amount is greater than the unlocked funds, whatever amount is unlocked will be sent to the depositor.

Afterwards, the deposit is marked as refunded. This is problematic because this amount can be less than what was deposited, and since the deposit is considered refunded, the rest of the funds will be irretrievable by the depositor.

Impact

Loss of depositor funds if attempting to refund

Code Snippet

the volume to be sent is calculated here
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/DepositManager/Implementations/DepositManagerV1.sol#L152-L181

The deposit is marked as refunded. A revert will occur when attempting to refund again.
https://github.com/sherlock-audit/2023-02-openq/blob/main/contracts/Bounty/Implementations/BountyCore.sol#L64-L76

Tool used

Manual Review

Recommendation

  • Consider only refunding full amounts
  • If a partial refund is needed, consider tracking the amount left to refund and subtracting from it after each partial refund.

Duplicate of #257

@github-actions github-actions bot added Has Duplicates A valid issue with 1+ other issues describing the same vulnerability High A valid High severity issue labels Feb 21, 2023
@FlacoJones
Copy link

Will fix by requiring funder == issuer. Will all be their own money, can simply refund up to the amount of token balance

@FlacoJones FlacoJones added Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed labels Feb 23, 2023
@FlacoJones
Copy link

@IAm0x52
Copy link
Collaborator

IAm0x52 commented Mar 6, 2023

Dupe of #257

@sherlock-admin sherlock-admin added Medium A valid Medium severity issue Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Reward A payout will be made for this issue and removed High A valid High severity issue Has Duplicates A valid issue with 1+ other issues describing the same vulnerability labels Mar 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Duplicate A valid issue that is a duplicate of an issue with `Has Duplicates` label Medium A valid Medium severity issue Reward A payout will be made for this issue Sponsor Confirmed The sponsor acknowledged this issue is valid Will Fix The sponsor confirmed this issue will be fixed
Projects
None yet
Development

No branches or pull requests

4 participants