Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Liquidity.sol#L121

Vulnerability details

Impact: In the function of Liquidity._withdrawLiquidityAndClaim(), removeLiquidity,safeTransfer(), _decreaseUserShare() would be inconsistent ,If Liquidity._withdrawLiquidityAndClaim() haven't try-catch block

Proof of Concept

Liquidity._withdrawLiquidityAndClaim() include three oprations:removeLiquidity,safeTransfer(), _decreaseUserShare(). We should add try-catch block to ensure consistency of the three oprations.

function _withdrawLiquidityAndClaim( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToWithdraw, uint256 minReclaimedA, uint256 minReclaimedB ) internal returns (uint256 reclaimedA, uint256 reclaimedB) { bytes32 poolID = PoolUtils._poolID( tokenA, tokenB ); require( userShareForPool(msg.sender, poolID) >= liquidityToWithdraw, "Cannot withdraw more than existing user share" );

    // Remove the amount of liquidity specified by the user.
    // The liquidity in the pool is currently owned by this contract. (external call)
    (reclaimedA, reclaimedB) = pools.removeLiquidity( tokenA, tokenB, liquidityToWithdraw, minReclaimedA, minReclaimedB, totalShares[poolID] );

    // Transfer the reclaimed tokens to the user
    tokenA.safeTransfer( msg.sender, reclaimedA );
    tokenB.safeTransfer( msg.sender, reclaimedB );

    _decreaseUserShare( msg.sender, poolID, liquidityToWithdraw, true );

    emit LiquidityWithdrawn(msg.sender, address(tokenA), address(tokenB), reclaimedA, reclaimedB, liquidityToWithdraw);
    }

Tools Used

vscode foundry

Recommended Mitigation Steps

add try-catch block.

Assessed type

Other

code-423n4 / 2024-01-salty-findings