#36450 [SC-Low] contract timebasedcollateralpool will be unable to process new user transactions

#36450 [SC-Low] Contract TimeBasedCollateralPool will be unable to process new user transactions and user funds are temporary frozen if a user unstake transaction of TimeBasedCollateralPool execute...

Submitted on Nov 3rd 2024 at 04:16:31 UTC by @perseverance for Audit Comp | Anvil

• Report ID: #36450

• Report Type: Smart Contract

• Report severity: Low

• Target: https://etherscan.io/address/0xd042C267758eDDf34B481E1F539d637e41db3e5a

• Impacts:

  • Temporary freezing of funds within the TimeBasedCollateralPool for at least 48 hours

Description

Description

Brief/Intro

The users can unstake from the TimeBasedCollateralPool by using the function unstake

```solidity function unstake(IERC20 _token, uint256 _poolUnits) external {} ```

The vulnerability

Vulnerability Details

The impact described in this bug report can happen in the Scenario:

If a victim user want to unstake his tokens from the pool, then he call ustake(). An user with RESETTER_ROLE reset the pool (may be via Governance Proposal Execution) and this transaction got executed before the victim's transaction. The victim's unstake transaction got executed after reset pool.

But the scenario can happen because of the race condition in Ethereum and the reset pool transaction can get executed before the user's unstake transaction. Because of decentralized and mempool, There is possibility that this scneario can happen.

Step 1: So the pool is reset.

Step 2: Now the victim's unstake transaction got executed

```solidity function unstake(IERC20 _token, uint256 _poolUnits) external { if (_poolUnits == 0) revert UnstakeAmountZero();

    address tokenAddress = address(_token);

701: if ( 702: accountTokenState[msg.sender][tokenAddress].totalUnits - 703: getTotalAccountUnitsPendingUnstake(msg.sender, tokenAddress) < 704: _poolUnits ) { revert InsufficientBalance( _poolUnits, accountTokenState[msg.sender][tokenAddress].totalUnits - getTotalAccountUnitsPendingUnstake(msg.sender, tokenAddress) ); }

    {
        // Release eligible account tokens. If a reset occurred, there is nothing left to unstake, so return.

715: bool accountWasReset = _releaseEligibleAccountTokens(msg.sender, tokenAddress); if (accountWasReset) { // Do not revert because the user wanted tokens unstaked, and those were unstaked and released. return; } }

    _addToAccountPendingUnstakeNextEpoch(tokenAddress, msg.sender, _poolUnits);
    _addToContractPendingUnstakeNextEpoch(tokenAddress, _poolUnits);

    emit UnstakeInitiated(msg.sender, _token, _poolUnits, getEpochEndTimestamp(getCurrentEpoch() + 1));
}

```

So after the reset pool event, when unstake(), at Line 701-704, the accountTokenState[msg.sender][tokenAddress].totalUnits is still the totalUnits of the user before the reset pool event.

But in Line 715 the internal function _releaseEligibleAccountTokens is called. In this internal function, if the pool was reset, then the tokens and units of the user are released.

```solidity function _releaseEligibleAccountTokens( address _account, address _tokenAddress ) internal returns (bool _poolWasReset) { _poolWasReset = _unlockEligibleTokenContractPendingUnstakes(_tokenAddress);

    (uint256 totalUnitsToRelease, uint256 totalTokensToRelease) = _resetAccountTokenStateIfApplicable(
        _account,
        _tokenAddress
    );

    if (totalUnitsToRelease == 0) {
        (uint256 units, uint256 tokens) = _processAccountTokenUnstakes(_account, _tokenAddress);
        totalUnitsToRelease += units;
        totalTokensToRelease += tokens;
    }

    if (totalUnitsToRelease == 0) {
        return _poolWasReset;
    }

    collateral.transferCollateral(_tokenAddress, totalTokensToRelease, _account);

    emit CollateralReleased(IERC20(_tokenAddress), totalTokensToRelease, totalUnitsToRelease, _account);
}

```

After the the tokens and units of the user are released, the user's totalUnits is reset to 0.

Please note that _releaseEligibleAccountTokens called inside unstake() still return false means that the pool was not reset because in this scenario, the reset happened before the unstake transaction.

Note: If the resetPool happened inside the _releaseEligibleAccountTokens function, then _releaseEligibleAccountTokens will return true.

But in the function unstake, the accountWasReset is false, so it still process unstake by calling

```solidity _addToAccountPendingUnstakeNextEpoch(tokenAddress, msg.sender, _poolUnits); _addToContractPendingUnstakeNextEpoch(tokenAddress, _poolUnits);

    emit UnstakeInitiated(msg.sender, _token, _poolUnits, getEpochEndTimestamp(getCurrentEpoch() + 1));

```

So now for the victim the state of accountTokenState is incorrect because

```solidity accountTokenState.totalUnits = 0 accountTokenState.firstPendingUnstakeUnits != 0 ``` The pool state is also incorrect

``` contractStateStorage.totalUnits = 0;

contractStateStorage.firstPendingUnstakeUnits != 0 and = unstake Units amount.

the ReserveCollateral of TimeBasedCollateralPool was also reset to 0

```

So the victim and other user future transactions after the willCompleteAtTimestampSeconds time, will call the modifier withEligibleAccountTokensReleased

```solidity modifier withEligibleAccountTokensReleased(address _account, address _tokenAddress) { _releaseEligibleAccountTokens(_account, _tokenAddress);

    _;
}

```

And this modifier will call function _unlockEligibleTokenContractPendingUnstakes

```solidity function _releaseEligibleAccountTokens( address _account, address _tokenAddress ) internal returns (bool _poolWasReset) { _poolWasReset = _unlockEligibleTokenContractPendingUnstakes(_tokenAddress); //... } ```

```solidity function _unlockEligibleTokenContractPendingUnstakes(address _tokenAddress) internal returns (bool) {

    // ... 

    if ((firstVestedUnits + secondVestedUnits) == totalPoolUnits) {
        collateral.releaseAllCollateral(contractStateStorage.collateralReservationId);
        contractStateStorage.collateralReservationId = 0;
    } else {
        uint256 tokensToRelease = firstVestedTokens + secondVestedTokens;
        if (tokensToRelease > 0) {
            try
                collateral.modifyCollateralReservation(
                    contractStateStorage.collateralReservationId,
                    -Pricing.safeCastToInt256(tokensToRelease)
                )
            {} catch (bytes memory reason) {
                if (bytes4(reason) == ICollateral.ClaimableAmountZero.selector) {
                    // If we're here, it means that the result of lowering the collateral amount is a reservation with
                    // 0 claimable balance. The only way to get this collateral out is to release less or more. Less
                    // invalidates the unstaking logic, so we choose to release more, resetting the dust that remains.
                    _resetPool(_tokenAddress);
                    return true;
                } else {
                    assembly {
                        revert(add(reason, 0x20), mload(reason))
                    }
                }
            }
        }
    }

    // ... 
}

```

Since the collateral.modifyCollateralReservation will revert in Line 748 in function _modifyCollateralReservation with the check because the oldReservedAmount was reset and the transaction want to release unstake amount.

if (byAmountUint >= oldReservedAmount) revert InsufficientCollateral(byAmountUint, oldReservedAmount); // @audit-issue

Note: In the POC, the victim or some other user stake small amount between the unstake transaction timestamp and the willCompleteAtTimestampSeconds, so the oldReservedAmount is not 0. But if no transactions happen during this time frame, then the oldReservedAmount is 0.

```solidity function _modifyCollateralReservation( uint96 _reservationId, int256 _byAmount ) internal returns (uint256 _reservedCollateral, uint256 _claimableCollateral) { // Redacted

    if (_byAmount < 0) {
        uint256 byAmountUint = uint256(-_byAmount);

Line 748: if (byAmountUint >= oldReservedAmount) revert InsufficientCollateral(byAmountUint, oldReservedAmount); // @audit-issue

        _reservedCollateral = oldReservedAmount - byAmountUint;
        reservationStorage.tokenAmount = _reservedCollateral;

        address account = reservationStorage.account;
        address tokenAddress = reservationStorage.tokenAddress;

        CollateralBalance storage balanceStorage = accountBalances[account][tokenAddress];
        balanceStorage.reserved -= byAmountUint;
        balanceStorage.available += byAmountUint;
    } else {
       
}

``` So all the transactions will revert in collateral.modifyCollateralReservation with the reason InsufficientCollateral

Since all user transactions will call the modifier withEligibleAccountTokensReleased, so all user transactions will revert.

Impacts

About the severity assessment

If the scenario in this bugs occur, then all of the victim and user future transactions after willCompleteAtTimestampSeconds of unstake transaction always revert. This will cause denial of service of the protocol and temporary frozen of funds in TimeBasedCollateralPool contract.

This will result that in the owner or the protocol team need to manualy interveen to rescue the funds and fix the situation, maybe to propose a Governance Proposal and execute it.

Bug Severity: Low

Impact Category: Temporary freezing of funds within the TimeBasedCollateralPool for at least 48 hours

Probability: Medium, likely

Because the protocol team will need to investigate the root cause and pass a proposal to fix this situation that might take 3-4 days so the freezing of funds can be longer than 48 hours.

Link to Proof of Concept

https://gist.github.com/Perseverancesuccess2021/f7643506a8ed7cc80ec111ce111151da#file-testtimebasedcollateralpool-sol

Proof of Concept

Proof of concept

Steps to reproduce the bug

Step 1: The user unstake his tokens

Step 2: the user with RESETTER ROLE reset the pool for some reasons and because of race condition, this transaction got executed before step 1

Test code to show: ```solidity function testUnStake() public {

    console.log("Get getAccountTokenState");
    TimeBasedCollateralPool.AccountState memory accountTokenState = TimeBasedCollateralPool(TimeBasedCollateralPool_contract).getAccountTokenState(victim, USDC); 
    console.log("accountTokenState: ", victim, accountTokenState.totalUnits, accountTokenState.firstPendingUnstakeEpoch);
    uint256 unstake_amount = accountTokenState.totalUnits; 
    console.log("Victim can unstake amount: ",unstake_amount);