Numoen contest - adeolu's results

Lines of code

https://github.com/code-423n4/2023-01-numoen/blob/2ad9a73d793ea23a25a381faadc86ae0c8cb5913/src/periphery/Payment.sol#L44

Vulnerability details

Impact

Anyone that calls the refundEth function in Payments contract in payments.sol can withdraw all the eth in contract. Note that payment contract is inherited by LiquidityManager and LendgineRouter contracts.

Proof of Concept

line to code in repo - https://github.com/code-423n4/2023-01-numoen/blob/2ad9a73d793ea23a25a381faadc86ae0c8cb5913/src/periphery/Payment.sol#L44

function refundETH() external payable { if (address(this).balance > 0) SafeTransferLib.safeTransferETH(msg.sender, address(this).balance); }

As seen in this refundEth fcn in Payment contract, the msg.sender which can be anyone can call this function and empty all eth in Payments contract, the Payments contract is inherited by the LiquidtyManager contract, which is used by a user to manage its positions, if a user deposits eth to enter a position via any of the other payable functions in the payments contract in payments.sol or if not all eth is used up while entering a position, at a later moment any other person can come and take the eth before it is used.

Recommended Mitigation Steps

This bug is similar to bug described by twitter user jeiwan7 in his blogpost here --> https://twitter.com/jeiwan7/status/1616981937293492224

I believe the best course of action should be calling refundETH() function in all functions where it is possible that not all eth will be used up when entering a position/swapping/trade, most especially in LiquidityManager contract and LendgineRouter contract.

This way makes it possible so the remaining unused eth is sent back to the user in a single function call.