Anyone can call addLiquidity function and be the router

[code423n4]

Lines of code

https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/models/Pool.sol#L166

Vulnerability details

Impact

Anyone can call addLiquidity, meaning that when IAddLiquidityCallback(msg.sender).addLiquidityCallback(tokenAAmount, tokenBAmount, data); is called, the msg.sender can be a contract that a malicious user has deployed and has a addLiquidityCallback(tokenAAmount, tokenBAmount, data) function that does nothing. This would mean the funds would not get transferred, so an actor could drain the pool.

Proof of Concept

1. Malicious actor creates a contract that has a function addLiquidityCallback, function claimERCTOkens and a function that can call pool's addLiquidity.
2. Malicious actor calls the function that calls pool's addLiquidity.
3. Since the pool will then call the malicious contract's addLiquidityCallback. no tokens will get sent.
4. The contract however will still send the contract tokens to the malicious contract.
5. By doing so the pool can be drained.

Tools Used

Manual review

Recommended Mitigation Steps

If the aim was to only allow router to call the function, set the router's address and add a modifier that checks if the caller is really the router.

[kirk-baird]

This issue is invalid as there is a balance check immediately after the call addLiquidityCallback(). Therefore if the required funds are not transferred the transaction will revert.

require(previousABalance + tokenAAmount <= _tokenABalance() && previousBBalance + tokenBAmount <= _tokenBBalance(), "A");

https://github.com/code-423n4/2022-12-Stealth-Project/blob/fc8589d7d8c1d8488fd97ccc46e1ff11c8426ac2/maverick-v1/contracts/models/Pool.sol#L168

[c4-judge]

kirk-baird marked the issue as unsatisfactory: Invalid