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

missing nonreentrant modfier #166

Open
code423n4 opened this issue Dec 15, 2021 · 2 comments
Open

missing nonreentrant modfier #166

code423n4 opened this issue Dec 15, 2021 · 2 comments
Labels
0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments)

Comments

@code423n4
Copy link
Contributor

Handle

certora

Vulnerability details

https://github.com/code-423n4/2021-12-sublime/blob/main/contracts/Pool/Pool.sol
missing non reentrant modifier in terminatePool and requestMarginCall and initialize.
all of the other functions of the contract have nonreentrant modifier therefore it's inconsistent.

Impact

There's a possibility for re-entrancy. Even though I did not find a way to exploit it, it seems like this function should have the nonReentrant modifier as the other pool functions have it as well.

Recommended Mitigation Steps

Add nonReentrant

A similar issue by cmichel was approved:
code-423n4/2021-10-union-findings#67

@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 Dec 15, 2021
code423n4 added a commit that referenced this issue Dec 15, 2021
@ritik99 ritik99 added the disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments) label Dec 27, 2021
@ritik99
Copy link
Collaborator

ritik99 commented Dec 27, 2021

initalize can only be called once, so it is inherently non-reentrant, and it is also privileged. terminatePool is also a privileged call, hence non-reentrant is not required. requestMarginCall doesn't have any surface for attack as it doesn't deal with any token related things. However, out of an abundance of caution, we will add a non-reentrancy guard to it. We suggest the severity be reduced to (0) non-critical

@0xean
Copy link
Collaborator

0xean commented Jan 21, 2022

lower to non critical as no attack vector found.

@0xean 0xean added 0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation and removed 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments labels Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 (Non-critical) Code style, clarity, syntax, versioning, off-chain monitoring (events etc), exclude gas optimisation bug Something isn't working disagree with severity Sponsor confirms validity, but disagrees with warden’s risk assessment (sponsor explain in comments)
Projects
None yet
Development

No branches or pull requests

3 participants