Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

first depositor of the insurance fund can drain the other depositors #116

Closed
code423n4 opened this issue Feb 23, 2022 · 0 comments
Closed
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate This issue or pull request already exists sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-02-hubble/blob/main/contracts/InsuranceFund.sol#L39

Vulnerability details

in deposit, when the ratio totalSupply / balance is very high, the amount of the minted shares can round down to zero.

Proof of Concept

Alice is the first one to deposit to the insurance fund.
she deposits 1 basic unit of vusd (10**-6 dollar), therefore minting one share.
then she transferred 1 million vusd to the contract.
then bob deposits 500,000 vusd. (500000 * 10**6 basic units)
the amount of shares he gets is 500000 * 10**6 * 1 / (1000000* 10**6) = 0
therefore the number of shares didn't change but the balance increased by 500000 dollars.
Alice can now withdraw her share and receive her funds back together with bob funds, as he doesn't have any shares.

Recommended Mitigation Steps

change to:

        if (_pool == 0) {
            shares = _amount * 10 ** 18;
        } else {
@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Feb 23, 2022
code423n4 added a commit that referenced this issue Feb 23, 2022
@atvanguard atvanguard added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Feb 24, 2022
@CloudEllie CloudEllie added the duplicate This issue or pull request already exists label Mar 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate This issue or pull request already exists sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

3 participants