search

Phoenix-Protocol-Group / phoenix-contracts

Source code of the smart contracts of the Phoenix DeFi hub DEX protocol
GNU General Public License v3.0
10 stars 7 forks source link

[V-PHX-VUL-014] Pools are vulnerable to tokens with hooks #214

Open gangov opened 9 months ago

gangov commented 9 months ago

file: contracts/pool/src/contract.rs location: do_swap, provide_liquidity, withdraw_liquidity

The do_swap, provide_liquidity and withdraw_liquidity functions transfer the funds to the recipient before updating the pool reserves. If one of the tokens involved includes a callback in its transfer function, then it can be used to reenter the pool and perform an operation with the pool reserves yet to be updated. Reference: https://uniswap.org/whitepaper.pdf —> go to section 3.3

Impact: As the update of pool reserves occurs subsequent to the token transfer, tokens equipped with a callback in their transfer function can potentially be used to reenter the pool before its reserves are updated. For example, they can swap again using the previous price.

Recommendation: Add reentrancy locks to the swap, provide_liquidity and withdraw_liquidity operations.

Reference: https://github.com/Uniswap/v2-core/blob/master/contracts/UniswapV2Pair. sol archive

gangov commented 8 months ago

from Stellar's Discord:

Reentrancy isn't implemented yet. At the moment it is disallowed. Once it is implemented, it'll be an explicit flag you have to specify to allow. — 05/30/2023 7:19 PM

last question on the topic was from two months later

gangov commented 8 months ago

found some extra on the topic

gangov commented 7 months ago

as per Discord discussion:

02/15/2024 3:04 PM hey, how are you all handling the protection against reentrancy attacks? There's a post about since last year, but nothing comes up after that

02/15/2024 6:32 PM contract reentrancy is (currently, but I think it'll remain like this) disabled in soroban's host environment

02/15/2024 7:01 PM thanks! So basically devs should not implement any additional checks for that type of attacks, is that correct?

02/15/2024 7:09 PM yes should always assume that any contract you're calling won't be able to call yours back within the same operation.

given all that information, should we put efforts trying to additionally protect against such attack?