A malicious user can borrow USDS without having to repay it.

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

Borrow USDS without having to repay it.

Proof of Concept

The CollateralAndLiquidity determines the amount of usds that can be lent based on the number of UserShareForpools.

  function borrowUSDS( uint256 amountBorrowed ) external nonReentrant {
        require( exchangeConfig.walletHasAccess(msg.sender), "Sender does not have exchange access" );
        require( userShareForPool( msg.sender, collateralPoolID ) > 0, "User does not have any collateral" );
        require( amountBorrowed <= maxBorrowableUSDS(msg.sender), "Excessive amountBorrowed" );
        .....
  }

Users use depositCollateralAndIncreaseShare increase userShare, withdrawCollateralAndClaim reduce userShare.

    function withdrawCollateralAndClaim(....) {
        require( userShareForPool( msg.sender, collateralPoolID ) > 0, "User does not have any collateral" );
@>      require( collateralToWithdraw <= maxWithdrawableCollateral(msg.sender), "Excessive collateralToWithdraw" );

        (reclaimedWBTC, reclaimedWETH) = _withdrawLiquidityAndClaim( wbtc, weth, collateralToWithdraw, minReclaimedWBTC, minReclaimedWETH );

        emit CollateralWithdrawn(msg.sender, collateralToWithdraw, reclaimedWBTC, reclaimedWETH);
    }

withdrawCollateralAndClaim, depending on the amount of usds lending decisions can withdraw the number of the userShare.

    function maxWithdrawableCollateral( address wallet ) public view returns (uint256){
        uint256 userCollateralAmount = userShareForPool( wallet, collateralPoolID );

        // If the user has no collateral then they can't withdraw any collateral
        if ( userCollateralAmount == 0 )
            return 0;

        // When withdrawing, require that the user keep at least the inital collateral ratio (default 200%)
@>      uint256 requiredCollateralValueAfterWithdrawal = ( usdsBorrowedByUsers[wallet] * stableConfig.initialCollateralRatioPercent() ) / 100;
        uint256 userCollateralValue = userCollateralValueInUSD( wallet );

        // If the user doesn't even have the minimum amount of required collateral then return zero
        if ( userCollateralValue <= requiredCollateralValueAfterWithdrawal )
            return 0;

        // The maximum withdrawable value in USD
        uint256 maxWithdrawableValue = userCollateralValue - requiredCollateralValueAfterWithdrawal;

        // Return the collateralAmount that can be withdrawn
        return userCollateralAmount * maxWithdrawableValue / userCollateralValue;
    }

The problem is that the CollateralAndLiquidity contract inherits Liquidity, which is available in the collateralandLiquidity contract via another public function, withdrawCollateral:

    function withdrawLiquidityAndClaim( IERC20 tokenA, IERC20 tokenB, uint256 liquidityToWithdraw, 
        uint256 minReclaimedA, uint256 minReclaimedB, uint256 deadline ) external nonReentrant 
        ensureNotExpired(deadline) returns (uint256 reclaimedA, uint256 reclaimedB)
        {
        require( PoolUtils._poolID( tokenA, tokenB ) != collateralPoolID, "Stablecoin collateral cannot be withdrawn via Liquidity.withdrawLiquidityAndClaim" );

        return _withdrawLiquidityAndClaim(tokenA, tokenB, liquidityToWithdraw, minReclaimedA, minReclaimedB);
        }
    }

Therefore, a malicious user can use Liquidity#withdrawLiquidityAndClaim to retrieve his collateral, so that he wouldn't have to pay any debts.

Tools Used

vscode, manual

Recommended Mitigation Steps

CollateralAndLiquidity contracts for withdrawLiquidityAndClaim function increase the access control.

Assessed type

Error

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid

[zhaojio]

I'm pretty sure this is a high serious bug, The key questions are: Users can retrieve their collateral using public methods in the parent class, This means that users can obtain usds without collateral.

Hope to check again.

[Picodes]

What about this line https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/Liquidity.sol#L159?