search

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

1 stars 0 forks source link

Unrestricted `addLiquidity` could cause unintended results on front-end apps that listen to events. #317

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

shw

Vulnerability details

Impact

The addLiquidity function in Pool.sol lacks an access control, which allows an attacker to add liquidity for any specific user. Front-end apps that listen to AddLiquidity events may be affected by this vulnerability and may go wrong since it is not the user's intent to add liquidity.

Proof of Concept

Referenced code: Pool.sol#L54-L75](https://github.com/code-423n4/2021-04-vader/blob/main/vader-protocol/contracts/Pools.sol#L54-L75)

PoC: Link to PoC See the file 302_addLiquidity.js for a PoC of this attack. To run it, use npx hardhat test 302_addLiquidity.js.

Tools Used

None

Recommended Mitigation Steps

Consider checking whether addLiquidity is called from the router. If not, then the transaction should revert. Add another function, e.g., addLiquidityDirectly, for end users if they want to interact with the pool to add liquidity directly.

strictly-scarce commented 3 years ago

Pools.sol is not designed to be interacted without a Router.

Nothing in the system relies on events, which are just used for metrics/UX

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

Bad UX experience aside, this would result in a user gaining funds, not losing them. It is definitely not high risk.