The vault account amount can be the result of an overflow

[code423n4]

Lines of code

https://github.com/code-423n4/2022-08-frax/blob/main/src/contracts/FraxlendPair.sol#L234-L263

Vulnerability details

Impact

The downcast uint128(_amountToTransfer) can result in an overflow, which would impact the _totalAsset.amout local variable, resulting in an incorrect amount for the totalAsset.amount state variable.

function withdrawFees(uint128 _shares, address _recipient) external onlyOwner returns (uint256 _amountToTransfer) {
    // Grab some data from state to save gas
    VaultAccount memory _totalAsset = totalAsset;
    VaultAccount memory _totalBorrow = totalBorrow;

    // Take all available if 0 value passed
    if (_shares == 0) _shares = uint128(balanceOf(address(this)));

    // We must calculate this before we subtract from _totalAsset or invoke _burn
    _amountToTransfer = _totalAsset.toAmount(_shares, true);

    // Check for sufficient withdraw liquidity
    uint256 _assetsAvailable = _totalAssetAvailable(_totalAsset, _totalBorrow);
    if (_assetsAvailable < _amountToTransfer) {
        revert InsufficientAssetsInContract(_assetsAvailable, _amountToTransfer);
    }

    // Effects: bookkeeping
    _totalAsset.amount -= uint128(_amountToTransfer);
    _totalAsset.shares -= _shares;

    // Effects: write to states
    // NOTE: will revert if _shares > balanceOf(address(this))
    _burn(address(this), _shares);
    totalAsset = _totalAsset;

    // Interactions
    assetContract.safeTransfer(_recipient, _amountToTransfer);
    emit WithdrawFees(_shares, _recipient, _amountToTransfer);
}

Proof of Concept

1. WithdrawFees receives a value bigger than the max value for uint128, e.g 340282366920938463463374607431768211455 + 1
2. The value of _totalAsset.amount will apply the decrement on the value resulted from the overflow, e.g. (340282366920938463463374607431768211455 + 1) = 0.
3. The field amount for the totalAsset state variable will persist an incorrect value, possibily smaller than intended.

Recommended Mitigation Steps

Apply downcasting using the safeCast library from OpenZeppelin.

$ git diff --no-index FraxlendPair.sol.orig FraxlendPair.sol
diff --git a/FraxlendPair.sol.orig b/FraxlendPair.sol
index d54b8f5..602135a 100644
--- a/FraxlendPair.sol.orig
+++ b/FraxlendPair.sol
@@ -28,6 +28,7 @@ pragma solidity ^0.8.15;
 import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
 import "@openzeppelin/contracts/security/ReentrancyGuard.sol";
+import "@openzeppelin/contracts/utils/math/SafeCast.sol";
 import "@chainlink/contracts/src/v0.8/interfaces/AggregatorV3Interface.sol";
 import "./FraxlendPairConstants.sol";
 import "./FraxlendPairCore.sol";
@@ -41,6 +42,7 @@ import "./interfaces/ISwapper.sol";
 contract FraxlendPair is FraxlendPairCore {
     using VaultAccountingLibrary for VaultAccount;
     using SafeERC20 for IERC20;
+    using SafeCast for uint256;

     constructor(
         bytes memory _configData,
@@ -249,7 +251,7 @@ contract FraxlendPair is FraxlendPairCore {
         }

         // Effects: bookkeeping
-        _totalAsset.amount -= uint128(_amountToTransfer);
+        _totalAsset.amount = _amountToTransfer.toUint128();
         _totalAsset.shares -= _shares;

         // Effects: write to states

[DrakeEvans]

Impossible for this scenario to occur. AmountToTransfer is calculated using _shares, which is the balance of the fees accumulated. It is impossible to accumulate enough fees such that _shares would be large enough to cause _amountToTransfer to be larger than uint128. In order for this to happen the contract would already have been broken with overflows of totalAsset.amount.

[gititGoro]

Fees are prevented from overflowing as per this line

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

This scenario is impossible. Marking invalid.