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

Unauthenticated remove liquidty function #99

Closed
code423n4 opened this issue Nov 29, 2021 · 4 comments
Closed

Unauthenticated remove liquidty function #99

code423n4 opened this issue Nov 29, 2021 · 4 comments
Labels
3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working duplicate This issue or pull request already exists invalid This doesn't seem right

Comments

@code423n4
Copy link
Contributor

Handle

ad3sh_

Vulnerability details

Impact

Anyone can call removeLiquidity function and remove the liquidity from contract
https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/DexHandlers/UniswapHandler.sol#L221

Contract is not validating or checking the users wallet LP
https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/DexHandlers/UniswapHandler.sol#L222

Attacker can drain the pool by calling remove liquidity

Proof of Concept

https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/DexHandlers/UniswapHandler.sol#L221
while removing the liquidty the function should validate the user's wallet address who is removing the liquidity

second uniswap- handler is calculating price through reservers it may lead price manipulation

Tools Used

Manual

Recommended Mitigation Steps

Add proper fix and validation make functions authentic
use chainlink and weighted geometric mean solution for pricing

@code423n4 code423n4 added 3 (High Risk) Assets can be stolen/lost/compromised directly bug Something isn't working labels Nov 29, 2021
code423n4 added a commit that referenced this issue Nov 29, 2021
@0xScotch 0xScotch added the duplicate This issue or pull request already exists label Dec 10, 2021
@0xScotch
Copy link
Collaborator

0xScotch commented Dec 10, 2021

#120

@0xScotch
Copy link
Collaborator

All above mentions are erroneous and have been corrected.

@GalloDaSballo
Copy link
Collaborator

The UniswapHandler is just a utility contract that deposits for the msg.sender and sends funds to it
Calling withdraw would just revert as the contract has no balance

@GalloDaSballo
Copy link
Collaborator

Marking as invalid

@GalloDaSballo GalloDaSballo added the invalid This doesn't seem right label Jan 10, 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 invalid This doesn't seem right
Projects
None yet
Development

No branches or pull requests

3 participants