Fraxlend (Frax Finance) contest - 0x1f8b's results

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/c4189a3a98b38c8c962c5ea72f1a322fbc2ae45f/src/contracts/FraxlendPairDeployer.sol#L405

Vulnerability details

Impact

The method globalPause is not tested and it doesn't work as expected.

Proof of Concept

Because the method returns an array (_updatedAddresses) and has never been initialized, when you want to set its value, it fails.

Recipe:

• Call globalPause with any valid address.
• The transaction will FAULT.

Affected source code

• FraxlendPairDeployer.sol#L405

Recommended Mitigation Steps

Initialize the _updatedAddresses array like shown bellow:

    function globalPause(address[] memory _addresses) external returns (address[] memory _updatedAddresses) {
        require(msg.sender == CIRCUIT_BREAKER_ADDRESS, "Circuit Breaker only");
        address _pairAddress;
        uint256 _lengthOfArray = _addresses.length;
+       _updatedAddresses = new address[](_lengthOfArray);
        for (uint256 i = 0; i < _lengthOfArray; ) {
            _pairAddress = _addresses[i];
            try IFraxlendPair(_pairAddress).pause() {
                _updatedAddresses[i] = _addresses[i];
            } catch {}
            unchecked {
                i = i + 1;
            }
        }
    }

• Low
  • 1. Outdated compiler
  • 2. SafeERC20 mismatch logics
  • 3. Lack of ACK during owner change
  • 4. Constant salt
  • 5. Bypasss TimeLock restrictions
  • 6. Division before multiple can lead to precision errors
  • 7. Ownable and Pausable
• Non-Critical
  • 8. Use encode instead of encodePacked for hashig
  • 9. Wrong comment of toBorrowAmount function
  • 10. Confusing variables as to how they are stored

Low

1. Outdated compiler

The pragma version used are:

pragma solidity ^0.8.15;

The minimum required version must be 0.8.16; otherwise, contracts will be affected by the following important bug fixes:

0.8.16

• Code Generation: Fix data corruption that affected ABI-encoding of calldata values represented by tuples: structs at any nesting level; argument lists of external functions, events and errors; return value lists of external functions. The 32 leading bytes of the first dynamically-encoded value in the tuple would get zeroed when the last component contained a statically-encoded array.

Apart from these, there are several minor bug fixes and improvements.

2. SafeERC20 mismatch logics

The SafeERC20 library says:

@title SafeERC20 provides helper functions for safe transfers as well as safe metadata access

But the reality is that metada access is not secure.

The SafeERC20.safeTransfer and SafeERC20.safeTransferFrom methods perform a safe check that the address to call is a contract by calling functionCall, this checks doesn't appear in the methods safeSymbol, safeName and safeDecimals, so some of the SafeERC20 library methods are prone to phantom methods attacks.

Proof of Concept:

If you try to call the SafeERC20 methods with the following token 0x000000000000000000000000000000000000000000000 (or any EOA) you can see that the transaction is successful, you have to check that the address is a valid contract and maybe check that the call returns a certain data. Otherwise it is possible to call it and lose tokens or produce undesired errors.

This remind me to this attack https://certik.medium.com/qubit-bridge-collapse-exploited-to-the-tune-of-80-million-a7ab9068e1a0 where we can see:

Affected source code:

• SafeERC20.sol#L39-L58

Recommended changes:

• Use functionStaticCall from OpenZeppelin.

+import "@openzeppelin/contracts/utils/Address.sol";
...
library SafeERC20 {
+    using Address for address;

    /// @notice Provides a safe ERC20.symbol version which returns '???' as fallback string.
    /// @param token The address of the ERC-20 token contract.
    /// @return (string) Token symbol.
    function safeSymbol(IERC20 token) internal view returns (string memory) {
-       (bool success, bytes memory data) = address(token).staticcall(abi.encodeWithSelector(SIG_SYMBOL));
+       (bool success, bytes memory data) = address(token).functionStaticCall(abi.encodeWithSelector(SIG_SYMBOL));
        return success ? returnDataToString(data) : "???";
    }

    /// @notice Provides a safe ERC20.name version which returns '???' as fallback string.
    /// @param token The address of the ERC-20 token contract.
    /// @return (string) Token name.
    function safeName(IERC20 token) internal view returns (string memory) {
-       (bool success, bytes memory data) = address(token).staticcall(abi.encodeWithSelector(SIG_NAME));
+       (bool success, bytes memory data) = address(token).functionStaticCall(abi.encodeWithSelector(SIG_NAME));
        return success ? returnDataToString(data) : "???";
    }

    /// @notice Provides a safe ERC20.decimals version which returns '18' as fallback value.
    /// @param token The address of the ERC-20 token contract.
    /// @return (uint8) Token decimals.
    function safeDecimals(IERC20 token) internal view returns (uint8) {
-       (bool success, bytes memory data) = address(token).staticcall(abi.encodeWithSelector(SIG_DECIMALS));
+       (bool success, bytes memory data) = address(token).functionStaticCall(abi.encodeWithSelector(SIG_DECIMALS));
        return success && data.length == 32 ? abi.decode(data, (uint8)) : 18;
    }

3. Lack of ACK during owner change

It's possible to lose the ownership under specific circumstances.

Because an human error it's possible to set a new invalid owner. When you want to change the owner's address it's better to propose a new owner, and then accept this ownership with the new wallet.

Affected source code:

• FraxlendWhitelist.sol#L30
• FraxlendPairDeployer.sol#L44

4. Constant salt

Is not possible to use public as pair name, if you are using the same default values as was used in deploy.

Becuase the salt for deploy is "public", is not possible to use the same as a name.

Affected source code:

• FraxlendPairDeployer.sol#L329
• FraxlendPairDeployer.sol#L372

Recommended changes:

-   keccak256(abi.encodePacked(_name)),
+   keccak256(abi.encodePacked("custom", _name)),

5. Bypasss TimeLock restrictions

Esto permite que llamadas como la que se muestran a continuacion, que se encuentran limitadas al contrato de TIME_LOCK_ADDRESS, sea posible ser llamadas por el owner, evitando cualquier limitación de tiempo, debido a que en primer lugar no se comprueba que la direccion establecida en setTimeLock sea un contrato, y segundo, porque podria establecer cualquier contrato, y en la misma llamada hacer la llamada limitada al TimeLock.

The FraxlendPair contract has a setTimeLock method that allows you to modify the TimeLock contract (TIME_LOCK_ADDRESS), this call is protected by the onlyOwner modifier.

This allows the owner to call methods like the one shown below (changeFee), which are limited to the TIME_LOCK_ADDRESS contract, avoiding any time constraints, since the established address in setTimeLock is not checked to be a contract, you could set any contract, and in the same transaction you can call the protected method.

    function changeFee(uint32 _newFee) external whenNotPaused {
        if (msg.sender != TIME_LOCK_ADDRESS) revert OnlyTimeLock();

This can facilitate rogue pool attacks.

Affected source code:

• FraxlendPair.sol#L204-L222

6. Division before multiple can lead to precision errors

Because Solidity integer division may truncate, it is often preferable to do multiplication before division to prevent precision loss.

Affected source code:

• FraxlendPairCore.sol#L315

Recommended changes:

    uint256 internal constant LTV_PRECISION = 1e5; // 5 decimals
    uint256 internal constant EXCHANGE_PRECISION = 1e18;
    ...
+   uint256 internal constant SOLVENT_PRECISION = EXCHANGE_PRECISION / LTV_PRECISION; // 1e13
    ... 

    function _isSolvent(address _borrower, uint256 _exchangeRate) internal view returns (bool) {
        if (maxLTV == 0) return true;
        uint256 _borrowerAmount = totalBorrow.toAmount(userBorrowShares[_borrower], true);
        if (_borrowerAmount == 0) return true;
        uint256 _collateralAmount = userCollateralBalance[_borrower];
        if (_collateralAmount == 0) return false;

-       uint256 _ltv = (((_borrowerAmount * _exchangeRate) / EXCHANGE_PRECISION) * LTV_PRECISION) / _collateralAmount;
+       uint256 _ltv = (_borrowerAmount * _exchangeRate) / (_collateralAmount * SOLVENT_PRECISION);
        return _ltv <= maxLTV;
    }

7. Ownable and Pausable

The contract FraxlendPairCore is Ownable and Pausable, so the owner could resign while the contract is paused, causing a Denial of Service. Owner resignation while the contract is paused should be avoided.

Affected source code:

• FraxlendPairCore.sol#L46

---

Non-Critical

8. Use encode instead of encodePacked for hashig

Use of abi.encodePacked is safe, but unnecessary and not recommended. abi.encodePacked can result in hash collisions when used with two dynamic arguments (string/bytes).

There is also discussion of removing abi.encodePacked from future versions of Solidity (ethereum/solidity#11593), so using abi.encode now will ensure compatibility in the future.

Affected source code:

• FraxlendPairDeployer.sol#L204

9. Wrong comment of toBorrowAmount function

The function toBorrowAmount is wrong commented.

Affected source code:

• FraxlendPair.sol#L187-L190

Recommended changes:

-   /// @notice The ```toBorrtoBorrowAmountowShares``` function converts a given amount of borrow debt into the number of shares
+   /// @notice The ```toBorrowAmount``` function converts a given number of shares into the amount of borrow debt
    /// @param _shares Shares of borrow
    /// @param _roundUp Whether to roundup during division
    function toBorrowAmount(uint256 _shares, bool _roundUp) external view returns (uint256) {
        return totalBorrow.toAmount(_shares, _roundUp);
    }

10. Confusing variables as to how they are stored

The FraxlendPairCore contract constructor takes two arguments, _configData and _immutables, however, there are immutables in _configData and variables in _immutables.

This may seem confusing, since for example one expects the TIME_LOCK_ADDRESS to be immutable as it is defined in the _immutables variable, however it is possible to change it with setTimeLock.

Affected source code:

• FraxlendPairCore.sol#L174
• FraxlendPairCore.sol#L190-L191
• FraxlendPairCore.sol#L193-L197

• Gas
  • 1. Don't use the length of an array for loops condition
  • 2. ++i costs less gas compared to i++ or i += 1
  • 3. There's no need to set default values for variables
    • Total gas saved: 8 * 6 = 48
  • 4. > 0 is less efficient than != 0 for unsigned integers
  • 5. Reduce the size of error messages Long revert Strings
    • Total gas saved: 18 * 8 = 144
  • 6. Use Custom Errors instead of Revert Strings to save Gas
    • Total gas saved: 9 * 9 = 81
  • 7. Optimize returnDataToString
  • 8. Cache abi.encodeWithSelector result
  • 9. Change bool to uint256 can save gas
  • 10. Use constants
  • 11. Gas saving using immutable
  • 12. Reuse abi.decode results
  • 13. Use immutable to cache constant data
  • 14. State variables must be cached in stack variables if they are reused
  • 15. Avoid compound assignment operator in state variables
    • Total gas saved: 13 * 7 = 91
  • 16. Reduce math operations

Gas

1. Don't use the length of an array for loops condition

It's cheaper to store the length of the array inside a local variable and iterate over it.

Affected source code:

• FraxlendPair.sol#L289
• FraxlendPair.sol#L308
• FraxlendPairCore.sol#L265
• FraxlendPairCore.sol#L270
• FraxlendWhitelist.sol#L51
• FraxlendWhitelist.sol#L66
• FraxlendWhitelist.sol#L81

2. ++i costs less gas compared to i++ or i += 1

++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.

i++ increments i and returns the initial value of i. Which means:

uint i = 1;
i++; // == 1 but i == 2

But ++i returns the actual incremented value:

uint i = 1;
++i; // == 2 and i == 2 too, so no need for a temporary variable

In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2 I suggest using ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--

Affected source code:

• FraxlendPair.sol#L289
• FraxlendPair.sol#L308
• FraxlendPairDeployer.sol#L130
• FraxlendPairDeployer.sol#L158
• FraxlendPairDeployer.sol#L408
• FraxlendWhitelist.sol#L51
• FraxlendWhitelist.sol#L66
• FraxlendWhitelist.sol#L81
• SafeERC20.sol#L24
• SafeERC20.sol#L27

3. There's no need to set default values for variables

If a variable is not set/initialized, the default value is assumed (0, false, 0x0 ... depending on the data type). You are simply wasting gas if you directly initialize it with its default value.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
 function testInit() public view returns (uint) { uint a = 0; return a; }
}

contract TesterB {
 function testNoInit() public view returns (uint) { uint a; return a; }
}

Gas saving executing: 8 per entry

TesterA.testInit:   21392
TesterB.testNoInit: 21384   

Affected source code:

• FraxlendPairConstants.sol#L47
• FraxlendPairDeployer.sol#L127
• FraxlendPairDeployer.sol#L152
• SafeERC20.sol#L22
• SafeERC20.sol#L27
• LinearInterestRate.sol#L33

Total gas saved: 8 * 6 = 48

4. > 0 is less efficient than != 0 for unsigned integers

Although it would appear that > 0 is less expensive than != 0, this is only true in the absence of the optimizer and outside of a need statement. Gas will be saved if the optimizer is enabled at 10k and you're in a require statement.

Reference:

• https://twitter.com/gzeon/status/1485428085885640706

Affected source code:

• LinearInterestRate.sol#L66

5. Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
 function testShortRevert(bool path) public view {
  require(path, "test error");
 }
}

contract TesterB {
 function testLongRevert(bool path) public view {
  require(path, "test big error message, more than 32 bytes");
 }
}

Gas saving executing: 18 per entry

TesterA.testShortRevert: 21886   
TesterB.testLongRevert:  21904       

Affected source code:

• FraxlendPairDeployer.sol#L205
• FraxlendPairDeployer.sol#L228
• FraxlendPairDeployer.sol#L253
• FraxlendPairDeployer.sol#L365
• FraxlendPairDeployer.sol#L366-L369
• LinearInterestRate.sol#L57-L60
• LinearInterestRate.sol#L61-L64
• LinearInterestRate.sol#L65-L68

Total gas saved: 18 * 8 = 144

6. Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source Custom Errors in Solidity:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
 function testRevert(bool path) public view {
  require(path, "test error");
 }
}

contract TesterB {
 error MyError(string msg);
 function testError(bool path) public view {
  if(path) revert MyError("test error");
 }
}

Gas saving executing: 9 per entry

TesterA.testRevert: 21611
TesterB.testError:  21602     

Affected source code:

• FraxlendPairDeployer.sol#L205
• FraxlendPairDeployer.sol#L228
• FraxlendPairDeployer.sol#L253
• FraxlendPairDeployer.sol#L365
• FraxlendPairDeployer.sol#L366-L369
• FraxlendPairDeployer.sol#L399
• LinearInterestRate.sol#L57-L60
• LinearInterestRate.sol#L61-L64
• LinearInterestRate.sol#L65-L68

Total gas saved: 9 * 9 = 81

7. Optimize returnDataToString

Since the value has already been checked to be != 0 previously, there is no need to do it again.

Affected source code:

• SafeERC20.sol#L27-L29

Recommended changes:

    function returnDataToString(bytes memory data) internal pure returns (string memory) {
        if (data.length >= 64) {
            return abi.decode(data, (string));
        } else if (data.length == 32) {
            uint8 i = 0;
            while (i < 32 && data[i] != 0) {
                i++;
            }
            bytes memory bytesArray = new bytes(i);
-           for (i = 0; i < 32 && data[i] != 0; i++) {
+           for (uint x; x < i; x++) {
-               bytesArray[i] = data[i];
+               bytesArray[x] = data[x];
            }
            return string(bytesArray);
        } else {
            return "???";
        }
    }

8. Cache abi.encodeWithSelector result

It is not necessary to call abi.encodeWithSelector with no data each time, it is cheaper to cache the byte result.

Affected source code:

• SafeERC20.sol#L14-L16

Recommended changes:

library SafeERC20 {
-   bytes4 private constant SIG_SYMBOL = 0x95d89b41; // symbol()
-   bytes4 private constant SIG_NAME = 0x06fdde03; // name()
-   bytes4 private constant SIG_DECIMALS = 0x313ce567; // decimals()
+   bytes private immutable SIG_SYMBOL = abi.encodeWithSelector(0x95d89b41); // symbol()
+   bytes private immutable SIG_NAME = abi.encodeWithSelector(0x06fdde03); // name()
+   bytes private immutable SIG_DECIMALS = abi.encodeWithSelector(0x313ce567); // decimals()
    ...
    function safeSymbol(IERC20 token) internal view returns (string memory) {
+       (bool success, bytes memory data) = address(token).staticcall(SIG_SYMBOL);
-       (bool success, bytes memory data) = address(token).staticcall(abi.encodeWithSelector(SIG_SYMBOL));
        return success ? returnDataToString(data) : "???";
    }
    ...
    function safeName(IERC20 token) internal view returns (string memory) {
+       (bool success, bytes memory data) = address(token).staticcall(SIG_NAME);
-       (bool success, bytes memory data) = address(token).staticcall(abi.encodeWithSelector(SIG_NAME));
        return success ? returnDataToString(data) : "???";
    }
    ...
    function safeDecimals(IERC20 token) internal view returns (uint8) {
+       (bool success, bytes memory data) = address(token).staticcall(SIG_DECIMALS);
-       (bool success, bytes memory data) = address(token).staticcall(abi.encodeWithSelector(SIG_DECIMALS));
        return success && data.length == 32 ? abi.decode(data, (uint8)) : 18;
    }
    ...

9. Change bool to uint256 can save gas

Because each write operation requires an additional SLOAD to read the slot's contents, replace the bits occupied by the boolean, and then write back, booleans are more expensive than uint256 or any other type that uses a complete word. This cannot be turned off because it is the compiler's defense against pointer aliasing and contract upgrades.

Reference:

• https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol#L23-L27

Affected source code:

• FraxlendWhitelist.sol#L32
• FraxlendWhitelist.sol#L35
• FraxlendWhitelist.sol#L38
• FraxlendPairDeployer.sol#L94
• FraxlendPairCore.sol#L78
• FraxlendPairCore.sol#L133
• FraxlendPairCore.sol#L134
• FraxlendPairCore.sol#L136
• FraxlendPairCore.sol#L137

10. Use constants

Use constant instead of storage/computed values in:

Affected source code:

• FraxlendPairDeployer.sol#L49-L51

11. Gas saving using immutable

It's possible to avoid storage access a save gas using immutable keyword for the following variables:

It's also better to remove the initial values, because they will be set during the constructor.

Affected source code:

• FraxlendPairDeployer.sol#L57-L59
• FraxlendPairCore.sol#L51

12. Reuse abi.decode results

deploy should decode _configData as an struct. In this way, we can avoid the third decoding in the _deployFirst, _deploySecond and _log methods.

Affected source code:

• FraxlendPairDeployer.sol#L249-L252
• FraxlendPairDeployer.sol#L288

struct ConfigData
{
    address _asset,
    address _collateral,
    address _oracleMultiply,
    address _oracleDivide,
    uint256 _oracleNormalization,
    address _rateContract,
    bytes memory _rateInitData
}
    function _deploySecond(
        string memory _name,
        address _pairAddress,
        ConfigData memory _configData,
        address[] memory _approvedBorrowers,
        address[] memory _approvedLenders
    ) private returns (ConfigData memory data) { ... }

    function _logDeploy(
        string memory _name,
        address _pairAddress,
        ConfigData memory _configData,
        uint256 _maxLTV,
        uint256 _liquidationFee,
        uint256 _maturityDate
    ) private { ... }

13. Use immutable to cache constant data

It is not necessary to continuously calculate the symbol, it can be cached during the contract deployment.

Affected source code:

• FraxlendPair.sol#L81

Recommended changes:

+   string private immutable _symbol;
+
    constructor(
        bytes memory _configData,
        bytes memory _immutables,
        uint256 _maxLTV,
        uint256 _liquidationFee,
        uint256 _maturityDate,
        uint256 _penaltyRate,
        bool _isBorrowerWhitelistActive,
        bool _isLenderWhitelistActive
    )
        FraxlendPairCore(
            _configData,
            _immutables,
            _maxLTV,
            _liquidationFee,
            _maturityDate,
            _penaltyRate,
            _isBorrowerWhitelistActive,
            _isLenderWhitelistActive
        )
        ERC20("", "")
        Ownable()
        Pausable()
-   {}
+   {
+       _symbol = string(abi.encodePacked("FraxlendV1 - ", collateralContract.safeSymbol(), "/", assetContract.safeSymbol()));
+   }

    function symbol() public view override(ERC20, IERC20Metadata) returns (string memory) {
        // prettier-ignore
        // solhint-disable-next-line max-line-length
-       return string(abi.encodePacked("FraxlendV1 - ", collateralContract.safeSymbol(), "/", assetContract.safeSymbol()));
+       return _symbol;
    }

14. State variables must be cached in stack variables if they are reused

It is possible to save accesses to storage by caching the state variables.

Affected source code:

• FraxlendPair.sol#L310-L312

Recommended changes:

    /// @notice The ```setApprovedBorrowers``` function sets a given array of addresses to the whitelist
    /// @dev Cannot black list self
    /// @param _borrowers The addresses whos status will be set
    /// @param _approval The approcal status
    function setApprovedBorrowers(address[] calldata _borrowers, bool _approval) external approvedBorrower {
        for (uint256 i = 0; i < _borrowers.length; i++) {
            // Do not set when _approval == false and _borrower == msg.sender
-           if (_approval || _borrowers[i] != msg.sender) {
-               approvedBorrowers[_borrowers[i]] = _approval;
-               emit SetApprovedBorrower(_borrowers[i], _approval);
+           address _borrower = _borrowers[i];
+           if (_approval || _borrower != msg.sender) {
+               approvedBorrowers[_borrower] = _approval;
+               emit SetApprovedBorrower(_borrower, _approval);
            }
        }
    }

15. Avoid compound assignment operator in state variables

Using compound assignment operator for state variables (like State += X or State -= X ...) it's more expensive than use operator assignment (like State = State + X or State = State - X ...).

Proof of concept (without optimizations):

pragma solidity 0.8.15;

contract TesterA {
 uint private _a;
 function testShort() public {
  _a += 1;
 }
}

contract TesterB {
 uint private _a;
 function testLong() public {
  _a = _a + 1;
 }
}

Gas saving executing: 13 per entry

TesterA.testShort: 43507
TesterB.testLong:  43494

Affected source code:

• FraxlendPairCore.sol#L724
• FraxlendPairCore.sol#L772
• FraxlendPairCore.sol#L773
• FraxlendPairCore.sol#L813
• FraxlendPairCore.sol#L815
• FraxlendPairCore.sol#L867
• FraxlendPairCore.sol#L1008

Total gas saved: 13 * 7 = 91

16. Reduce math operations

It is possible to reduce the number of operations inside _isSolvent by creating a new constant with value 1e13. We can now divide by just this constant instead of multiplying by 1e18 and dividing by 1e5, with the loss of precision that this causes.

Affected source code:

• FraxlendPairCore.sol#L315

Recommended changes:

    uint256 internal constant LTV_PRECISION = 1e5; // 5 decimals
    uint256 internal constant EXCHANGE_PRECISION = 1e18;
    ...
+   uint256 internal constant SOLVENT_PRECISION = EXCHANGE_PRECISION / LTV_PRECISION; // 1e13
    ... 

    function _isSolvent(address _borrower, uint256 _exchangeRate) internal view returns (bool) {
        if (maxLTV == 0) return true;
        uint256 _borrowerAmount = totalBorrow.toAmount(userBorrowShares[_borrower], true);
        if (_borrowerAmount == 0) return true;
        uint256 _collateralAmount = userCollateralBalance[_borrower];
        if (_collateralAmount == 0) return false;

-       uint256 _ltv = (((_borrowerAmount * _exchangeRate) / EXCHANGE_PRECISION) * LTV_PRECISION) / _collateralAmount;
+       uint256 _ltv = (_borrowerAmount * _exchangeRate) / (_collateralAmount * SOLVENT_PRECISION);
        return _ltv <= maxLTV;
    }