#36309 [SC-Low] TimeBasedCollateralPool: After _resetPool gets called (internally) a depositor can break most functionalities of the smart contract

Submitted on Oct 28th 2024 at 22:48:59 UTC by @max10afternoon for Audit Comp | Anvil

• Report ID: #36309

• Report Type: Smart Contract

• Report severity: Low

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

• Impacts:

  • Griefing (e.g. no profit motive for an attacker, but damage to the users or the protocol)

Description

Brief/Intro

After a pool gets reset, a depositor can call unstake to intentionally (or unintentionally) break almost all functionalities of the smart contract, including staking and unstaking. Leading to a freezing of funds in cases were and ExitBalance is present, or to a general disruption of service otherwise.

Vulnerability Details

When a pool gets reset the associated reservation ID gets set to 0, together with any pending epoch: ``` function _resetPool(address _tokenAddress) internal { // Note: we are not calling _unlockEligibleTokenContractPendingUnstakes because it can call this function.

    ContractState storage contractStateStorage = tokenContractState[_tokenAddress];

    uint256 unitsToReset = contractStateStorage.totalUnits;
    if (unitsToReset == 0) {
        // This already has the state that a reset would achieve, so it's not required.
        return;
    }

    // NB: must be resetNonce++, NOT ++resetNonce
    uint256 resetNonce = contractStateStorage.resetNonce++;

    uint256 tokensToReset;
    {
        uint96 reservationId = contractStateStorage.collateralReservationId;
        if (reservationId != 0) {
            // Unlock all pool tokens so they are releasable.
            tokensToReset = collateral.releaseAllCollateral(reservationId);
            contractStateStorage.collateralReservationId = 0;
        }
    }

    // Only set an Exit balance if there is one. If all tokens were claimed, then effectively set (0,0).
    if (tokensToReset > 0) {
        // Create the reset ExitBalance so stakers can exit their tokens (see: _resetAccountTokenStateIfApplicable(...))
        tokenResetExitBalances[_tokenAddress][resetNonce] = ExitBalance(unitsToReset, tokensToReset);
    }

    // Delete all contract-level pending unstake state.
    if (contractStateStorage.firstPendingUnstakeEpoch > 0) {
        contractStateStorage.firstPendingUnstakeEpoch = 0;
        contractStateStorage.firstPendingUnstakeUnits = 0;

        if (contractStateStorage.secondPendingUnstakeEpoch > 0) {
            contractStateStorage.secondPendingUnstakeEpoch = 0;
            contractStateStorage.secondPendingUnstakeUnits = 0;
        }
    }

    contractStateStorage.totalUnits = 0;

    emit PoolReset(IERC20(_tokenAddress), resetNonce + 1, tokensToReset, unitsToReset);
}

``` This two facts will be relevant inside of the '_unlockEligibleTokenContractPendingUnstakes', which gets called by all stake functionalities, unstake and claim. As calling unstake after the pool got reset will cause _unlockEligibleTokenContractPendingUnstakes to revert braking all of the above functionalities, here is why:

As you may notice at the begging, if uint256 firstEpoch = contractStateStorage.firstPendingUnstakeEpoch; is 0, no further computations will be made, avoiding any unexpected revert, that would come from interacting with the underlying Collateral contract trough a reservation with ID equal to 0. That said a user can call unstake to froce the contractStateStorage.firstPendingUnstakeEpoch to be greater than 0**, meaning that after the epoch elapses, the above mentioned check will pass and, unless someone have deposited before the time has passed, every functionality dependent on _unlockEligibleTokenContractPendingUnstakes will revert, breaking almost every functionalities of the contract with respect to the targeted token and freezing any funds found in a potential ExitBalance allocation until the implementation gets re deployed (with the owner's time lock currently having a min delay of 7 days).

**Note that the check performed inside of unstake, to verify if the account was reset will be ignored, as when epoch is equal to 0, the _poolWasReset variable will be set to false, meaning that accountWasReset will also be set to false, allowing the execution to reach the critical _addToContractPendingUnstakeNextEpoch function.

All of this can be done maliciously do damage the users and owners of the protocol (even by frontrunning the pool's reset with a dust amount deposit, see PoC) or by mistake by a user trying to legitimately call unstake.

Impact Details

Griefing: A malicious user can decide to break most functionalities of the contract either by having some funds staked already of by frontrunning (for almost no cost). Also this might lead to a temporary freezing of funds in cases were an ExitBalance was allocated.

Proof of Concept

Here is a simple foundry PoC interacting directly with the in scope assets (0xd042C267758eDDf34B481E1F539d637e41db3e5a) after properly initializing it, any interaction trough a proxy would lead to the same results.

Place your alchemy API key in the URL on line 119 of the script: ``` // SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.0;

import "forge-std/Test.sol"; import "forge-std/console.sol";

interface IStruct{

struct CollateralizableContractApprovalConfig {
    address collateralizableAddress;
    bool isApproved;
}


struct AccountState {
    uint32 resetNonce;
    uint32 firstPendingUnstakeEpoch;
    uint32 secondPendingUnstakeEpoch;
    uint256 firstPendingUnstakeUnits;
    uint256 secondPendingUnstakeUnits;
    uint256 totalUnits;
}

} interface IERC20 {

event Transfer(address indexed from, address indexed to, uint256 value);

event Approval(address indexed owner, address indexed spender, uint256 value);


function totalSupply() external view returns (uint256);

function balanceOf(address account) external view returns (uint256);

function transfer(address to, uint256 value) external returns (bool);

function allowance(address owner, address spender) external view returns (uint256);

function approve(address spender, uint256 value) external returns (bool);

function transferFrom(address from, address to, uint256 value) external returns (bool);

}

interface ICollateral is IStruct{

function depositAndApprove(
    address[] calldata _tokenAddresses,
    uint256[] calldata _amounts,
    address _collateralizableContractAddressToApprove
) external ;

function upsertCollateralizableContractApprovals(
    CollateralizableContractApprovalConfig[] calldata _updates
) external;

}

interface ITimeBasedCollateralPool is IStruct {

function defaultClaimDestinationAccount() external view returns (address);

function initialize(
    ICollateral _collateral,
    uint256 _epochPeriodSeconds,
    address _defaultClaimDestination,
    address _admin,
    address _claimant,
    address _claimRouter,
    address _resetter
) external;

function stake(
    IERC20 _token,
    uint256 _amount,
    bytes calldata _collateralizableApprovalSignature
) external;


function unstake(IERC20 _token, uint256 _poolUnits) external;

function getAccountTokenState(address _account, address _tokenAddress) external view returns (AccountState memory);