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

Reentrancy in withdraw function #148

Closed
code423n4 opened this issue Feb 9, 2022 · 2 comments
Closed

Reentrancy in withdraw function #148

code423n4 opened this issue Feb 9, 2022 · 2 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate This issue or pull request already exists

Comments

@code423n4
Copy link
Contributor

Lines of code

https://github.com/code-423n4/2022-02-concur/blob/main/contracts/Shelter.sol#L52-L59

Vulnerability details

Description

The contract was found vulnerable to Reentrancy attack. It was noticed that the function withdraw makes an external call to another untrusted address or a contract before it resolves any effects at line https://github.com/code-423n4/2022-02-concur/blob/main/contracts/Shelter.sol#L57

If the attacker controls the untrusted contract, they may be able to call back to the original function, repeating interactions that would have otherwise not run after the effects were resolved.

Impact

Reentrancy vulnerabilities can lead to various critical outcomes such as token stealing and burning. Adversaries may exploit the bug to mint tokens without any limitations or extract all the tokens out of the contract.

  • This vulnerability may lead to withdrawal token more than once.

PoC

Check the withdraw function and we will notice it is missing nonReentrant modifier.
https://github.com/code-423n4/2022-02-concur/blob/main/contracts/Shelter.sol#L52-L59

Recommended Mitigation Steps

Introduce a modifier nonreentrant to prevent Reentrancy vulnerabilities by implementing a Check-Effects-Interactions pattern.

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Feb 9, 2022
code423n4 added a commit that referenced this issue Feb 9, 2022
@r2moon r2moon added the duplicate This issue or pull request already exists label Feb 18, 2022
@r2moon
Copy link
Collaborator

r2moon commented Feb 18, 2022

duplicated with #260

@GalloDaSballo
Copy link
Collaborator

Duplicate of #260
But written as a low quality copy-paste

@GalloDaSballo GalloDaSballo added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value and removed 3 (High Risk) Assets can be stolen/lost/compromised directly labels Apr 19, 2022
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 Something isn't working duplicate This issue or pull request already exists
Projects
None yet
Development

No branches or pull requests

4 participants