Unauthorized User Access for Depositing/Withdrawing Liquidity in Pools

[c4-bot-1]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Liquidity.sol#L146 https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Liquidity.sol#L157

Vulnerability details

Impact

In the Liquidity.sol contract, as per the documentation, users are supposed to deposit liquidity into the liquidity pools and receive SALT rewards. However, there is a discrepancy between Liquidity::depositLiquidityAndIncreaseShare and CollateralAndLiquidity::depositCollateralAndIncreaseShare. Specifically, using the former does not permit depositing the poolID of WBTC/WETH collateral.

When users attempt to add liquidity in Liquidity::depositLiquidityAndIncreaseShare with tokens other than WBTC/WETH, Liquidity::_depositLiquidityAndIncreaseShare is called, which in turn calls Pools::addLiquidity. But, due to the restriction in Pools::addLiquidity, users are unable to complete this transaction:

    function addLiquidity(
        IERC20 tokenA,
        IERC20 tokenB,
        uint256 maxAmountA,
        uint256 maxAmountB,
        uint256 minLiquidityReceived,
        uint256 totalLiquidity
    ) external nonReentrant returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity) {
@>        require(msg.sender == address(collateralAndLiquidity), "Pools.addLiquidity is only callable from the CollateralAndLiquidity contract"
        );

Similarly, Liquidity::withdrawLiquidityAndClaim is affected by the same underlying issue.

These are the initial pairs affected in the whitelist: -SALT/WBTC -SALT/WETH -SALT/USDS -WBTC/USDS -WBTC/DAI -WETH/USDS -WETH/DAI -USDS/DAI

Proof of Concept

Consider the following function in Liquidity.sol, which demonstrates the issue:

    function depositLiquidityAndIncreaseShare(
        IERC20 tokenA,
        IERC20 tokenB,
        uint256 maxAmountA,
        uint256 maxAmountB,
        uint256 minLiquidityReceived,
        uint256 deadline,
        bool useZapping
    )
        external
        nonReentrant
        ensureNotExpired(deadline)
        returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity)
    {
        require(
            PoolUtils._poolID(tokenA, tokenB) != collateralPoolID,
            "Stablecoin collateral cannot be deposited via Liquidity.depositLiquidityAndIncreaseShare"
        );

        return
@>            _depositLiquidityAndIncreaseShare(tokenA, tokenB, maxAmountA, maxAmountB, minLiquidityReceived, useZapping);
    }

......
......
......
    function _depositLiquidityAndIncreaseShare(
        IERC20 tokenA,
        IERC20 tokenB,
        uint256 maxAmountA,
        uint256 maxAmountB,
        uint256 minLiquidityReceived,
        bool useZapping
    ) internal returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity) {
        require(exchangeConfig.walletHasAccess(msg.sender), "Sender does not have exchange access");

        // Transfer the specified maximum amount of tokens from the user
        tokenA.safeTransferFrom(msg.sender, address(this), maxAmountA);
        tokenB.safeTransferFrom(msg.sender, address(this), maxAmountB);

        if (useZapping) {
            (maxAmountA, maxAmountB) = _dualZapInLiquidity(tokenA, tokenB, maxAmountA, maxAmountB);
        }

        // Approve the liquidity to add
        tokenA.approve(address(pools), maxAmountA);
        tokenB.approve(address(pools), maxAmountB);

        // Deposit the specified liquidity into the Pools contract
        // The added liquidity will be owned by this contract. (external call to Pools contract)
        bytes32 poolID = PoolUtils._poolID(tokenA, tokenB);
@>        (addedAmountA, addedAmountB, addedLiquidity) =
            pools.addLiquidity(tokenA, tokenB, maxAmountA, maxAmountB, minLiquidityReceived, totalShares[poolID]); 
...
...
...

./src/staking/Liquidity.sol

function addLiquidity(
        IERC20 tokenA,
        IERC20 tokenB,
        uint256 maxAmountA,
        uint256 maxAmountB,
        uint256 minLiquidityReceived,
        uint256 totalLiquidity
    ) external nonReentrant returns (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity) {
        require(
@>            msg.sender == address(collateralAndLiquidity),
            "Pools.addLiquidity is only callable from the CollateralAndLiquidity contract"
        );

./src/pools/Pools.sol

Tools Used

Manual code review

Recommended Mitigation Steps

Adjust the access control in Pools.sol to allow Liquidity.sol to call addLiquidity. Ensure all other functionalities are working correctly after this modification.

Create a suit test for Liquidity.sol functionality, since the given test does not include it

Assessed type

Access Control

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[Picodes]

CollateralAndLiquidity inherits Liquidity