#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();
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) ); }
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; } }
```
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);
```
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);
```
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) {
```
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
Line 748: if (byAmountUint >= oldReservedAmount) revert InsufficientCollateral(byAmountUint, oldReservedAmount); // @audit-issue
``` 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 {