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

Basket becomes unusable if everybody burns their shares #64

Open
code423n4 opened this issue Sep 20, 2021 · 1 comment
Open

Basket becomes unusable if everybody burns their shares #64

code423n4 opened this issue Sep 20, 2021 · 1 comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Warden finding 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

Handle

kenzo

Vulnerability details

While handling the fees, the contract calculates the new ibRatio by dividing by totalSupply. This can be 0 leading to a division by 0.

Impact

If everybody burns their shares, in the next mint, totalSupply will be 0, handleFees will revert, and so nobody will be able to use the basket anymore.

Proof of Concept

Vulnerable line:
https://github.com/code-423n4/2021-09-defiProtocol/blob/52b74824c42acbcd64248f68c40128fe3a82caf6/contracts/contracts/Basket.sol#L124
You can add the following test to Basket.test.js and see that it reverts:
it("should divide by 0", async () => {
await basket.connect(addr1).burn(await basket.balanceOf(addr1.address));
await basket.connect(addr2).burn(await basket.balanceOf(addr2.address));

  await UNI.connect(addr1).approve(basket.address, ethers.BigNumber.from(1));
  await COMP.connect(addr1).approve(basket.address, ethers.BigNumber.from(1));
  await AAVE.connect(addr1).approve(basket.address, ethers.BigNumber.from(1));
  await basket.connect(addr1).mint(ethers.BigNumber.from(1));

});

Tools Used

Manual analysis, hardhat.

Recommended Mitigation Steps

Add a check to handleFees: if totalSupply= 0, you can just return, no need to calculate new ibRatio / fees.
You might want to reset ibRatio to BASE at this point.

@code423n4 code423n4 added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Warden finding labels Sep 20, 2021
code423n4 added a commit that referenced this issue Sep 20, 2021
@frank-beard frank-beard added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Oct 19, 2021
@GalloDaSballo
Copy link
Collaborator

I feel like this can also happen right after the contract was created
mintTo will call handleFees which will revert due to division by 0

Finding is valid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Warden finding 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