search

code-423n4 / 2021-04-vader-findings

1 stars 0 forks source link

Users may unintendedly remove liquidity under a phishing attack. #316

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

shw

Vulnerability details

Impact

The removeLiquidity function in Pools.sol uses tx.origin to determine the person who wants to remove liquidity. However, such a design is dangerous since the pool assumes that this function is called from the router, which may not be true if the user is under a phishing attack, and he could unintendedly remove liquidity.

Proof of Concept

Referenced code: Pool.sol#L77-L79

Tools Used

None

Recommended Mitigation Steps

Consider making the function _removeLiquidity external, which can be utilized by the router, providing information of which person removes his liquidity.

strictly-scarce commented 3 years ago

If a user has been phished, consider all their funds already stolen.

Vader's security assumption is a user is not phished.

Mervyn853 commented 3 years ago

Our decision matrix for severity:

0: No-risk: Code style, clarity, off-chain monitoring (events etc), exclude gas-optimisations 1: Low Risk: UX, state handling, function incorrect as to spec 2: Funds-Not-At-Risk, but can impact the functioning of the protocol, or leak value with a hypothetical attack path with stated assumptions, but external requirements 3: Funds can be stolen/lost directly, or indirectly if a valid attack path shown that does not have handwavey hypotheticals.

Recommended: 0

dmvt commented 3 years ago

This is reasonably easy to mitigate as an issue and failure to do so does leave an attack vector open. If exploited it will result in a loss of user funds.