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.

Sign up for GitHub

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

Jump to bottom

Missing validation check in totalSupply() #170

Open
code423n4 opened this issue Feb 2, 2022 · 1 comment
Open

Missing validation check in totalSupply() #170

code423n4 opened this issue Feb 2, 2022 · 1 comment
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working 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

SolidityScan

Vulnerability details

Description

The value of totalSupply() at https://github.com/code-423n4/2022-01-notional/blob/main/contracts/sNOTE.sol#L260
does not check if the value of totalSupply is 0 or not and it is per

Impact

The return value for the function getPoolTokenShare can be invalid because if there's an error in the totalSupply() the code at Line 260 will evaluate to divide by zero creating inconsistencies in the function logic.

Proof of Concept

  1. Check the function at https://github.com/code-423n4/2022-01-notional/blob/main/contracts/sNOTE.sol#L257-L261
  2. At line 260 we will notice that the value of totalSupply() is directly being used to perform division to the multiplication of bptBalance * sNOTEAmount

Recommended Mitigation Steps

Add a check if the value of totalSupply() is zero or not or some other edge cases that can cause inconsistencies.
@code423n4 code423n4 added 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working labels Feb 2, 2022
code423n4 added a commit that referenced this issue Feb 2, 2022
@code423n4
SolidityScan issue #170
59c5655
@jeffywu jeffywu mentioned this issue Feb 6, 2022
Closed
@jeffywu jeffywu added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Feb 6, 2022
@jeffywu jeffywu mentioned this issue Feb 6, 2022
Closed
jeffywu added a commit to notional-finance/staked-note that referenced this issue Feb 9, 2022
@jeffywu
Adding totalSupply check [code-423n4/2022-01-notional-findings#170]
902d3cd
jeffywu added a commit to notional-finance/staked-note that referenced this issue Feb 9, 2022
@jeffywu
Adding totalSupply check [code-423n4/2022-01-notional-findings#170]
87653d1
@pauliax
Copy link
Collaborator

pauliax commented Feb 14, 2022

Valid finding.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@jeffywu @pauliax @code423n4