[LOW-01] - checkBalances in commitCollateral is triggered without short-circuiting, allowing borrowers to commit collateral even if they don't own it

[ethereumdegen]

0xAdri:

In commitCollateral, checkBalances is called to ensure the borrower owns the collateral. However, checkBalances triggers the internal _checkBalances hardcoding false as the _shortCircut parameter:

// File: CollateralManager.sol

function checkBalances( address _borrowerAddress, Collateral[] calldata collateralInfo ) public returns (bool validated, bool[] memory checks_) { return _checkBalances(_borrowerAddress, _collateralInfo, false); } Because of this, borrowers can submit multiple collaterals they don't own, and as long as the last collateral is actually owned by them, all collaterals will be correctly committed due to the logic in _checkBalances:

// File: CollateralManager.sol

function _checkBalances( address _borrowerAddress, Collateral[] memory _collateralInfo, bool shortCircut ) internal virtual returns (bool validated, bool[] memory checks) { checks = new bool; validated_ = true; for (uint256 i; i < _collateralInfo.length; i++) { bool isValidated = _checkBalance( _borrowerAddress, collateralInfo[i] ); checks[i] = isValidated; if (!isValidated) { validated_ = false; //if short circuit is true, return on the first invalid balance to save execution cycles. Values of checks[] will be invalid/undetermined if shortcircuit is true. if (shortCircut) { return (validated, checks_); } } } }

However, note this is not critical, as accepting such a bid will fail due trying to transfer assets that are not owned by the borrower.

Recommendation Force the whole commitCollateral function to only commit collateral if all the passed collateral is owned by the borrower.

[ethereumdegen]

I dont think this is an issue at all because shortCircuit = false is actually the more strict / stringent and safer case.

 function _checkBalances(
        address _borrowerAddress,
        Collateral[] memory _collateralInfo,
        bool _shortCircut
    ) internal virtual returns (bool validated_, bool[] memory checks_) {
        checks_ = new bool[](_collateralInfo.length);
        validated_ = true;
        for (uint256 i; i < _collateralInfo.length; i++) {
            bool isValidated = _checkBalance(
                _borrowerAddress,
                _collateralInfo[i]
            );
            checks_[i] = isValidated;
            if (!isValidated) {
                validated_ = false;
                //if short circuit is true, return on the first invalid balance to save execution cycles. Values of checks[] will be invalid/undetermined if shortcircuit is true.
                if (_shortCircut) {
                    return (validated_, checks_);
                }
            }
        }
    }

Note that when short circuit is false, all assets have to have a valid balance.  if ANY asset in the loop  has an invalid balance, validated becomes false and can never be reset to true for any reason.