Deivitto - Reentrancy can drain liquidity `Pool`s

[github-actions[bot]]

Deivitto

high

Reentrancy can drain liquidity Pools

Summary

The scope says it doesn't use ERC777, however, says it uses ANY ERC20 (all in scape except rebasing tokens), tokens that implement ERC20 can easily implement ERC777 as it is backwards compatible, therefore incidents as the imBTC one where wasn't expected compatibility but however, users where affected by it and lost the assets.

Also in theory contract follows CEI (Check-interactions-effects). However at first line of each of the functions of Pool.sol that affect assets a call to a balanceOf (user created by pool deployer). This call to balanceOf(address(this)) is also a called that is affected by transfers not due to regular deposits, but to transferFrom to this contract

Vulnerability Detail

Code uses instances of ERC20s (expected tokens) transfer method

safeTransfer(LOAN_TOKEN, msg.sender, amount);
safeTransfer(COLLATERAL_TOKEN, msg.sender, amount);

what in a ERC777 can reenter due to hooks and avoid CEI pattern (what is neither followed in functions that affect assets as they call uint _loanTokenBalance = LOAN_TOKEN.balanceOf(address(this));. LOAN_TOKEN and COLLATERAL_TOKEN are input token for the Pair, a malicious (or just someone without knowledge about smart contracts) Pool creator can create an scenario that can be exploited to drain Pools where liquidity is provided.

Impact

Loss of assets

Code Snippet

https://github.com/Surge-fi/surge-protocol-v1//blob/3f77f58211b8f3257c81cdf61785dce9c17b318a/src/Pool.sol#L450 https://github.com/Surge-fi/surge-protocol-v1//blob/3f77f58211b8f3257c81cdf61785dce9c17b318a/src/Pool.sol#L455-L498 https://github.com/Surge-fi/surge-protocol-v1//blob/3f77f58211b8f3257c81cdf61785dce9c17b318a/src/Pool.sol#L504-L547 https://github.com/Surge-fi/surge-protocol-v1//blob/3f77f58211b8f3257c81cdf61785dce9c17b318a/src/Pool.sol#L396

Tool used

Manual Review

Recommendation

Create a whitelist, avoid reentrancy in erc777 tokens