Active Boosts

Boost _ Folks Finance 33970 - [Smart Contract - Medium] User deposits can be blocked

Submitted on Sat Aug 03 2024 01:56:26 GMT-0400 (Atlantic Standard Time) by @JCN2023 for Boost | Folks Finance

Report ID: #33970

Report type: Smart Contract

Report severity: Medium

Target: https://testnet.snowtrace.io/address/0x2cAa1315bd676FbecABFC3195000c642f503f1C9

Impacts:

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

Description

Brief/Intro

Currently there are two loanTypeIds (1, 2) for each poolId. Therefore, there are two loanPools available for each pool, since a loanPool is defined as loanPool = (loanTypeId, poolId). Activity will vary across the loanPools for a specific poolId and thus the collateralRewardIndex for these loanPools will vary (one will be greater than the other). At the time of writing this report, loanTypeId 1 for poolIds 128 & 129 (USDC & AVAX) point to a loanPool = (1, 128/129) that has a smaller collateralRewardIndex compared to loanPool = (2, 128/129). This, coupled with the fact that loanIds are reusable (can be created and deleted) and can hold values from previous usage, allows a bad actor to block users from depositing into certain pools via the following actions:

  1. Create loan using user's specified loanId (seen in mempool), but specifying a loanTypeId that points to a loanPool that has a larger collateralRewardIndex

  2. Deposit into loanPool so that the local _userLoans[loanId].collaterals[poolId].rewardIndex gets set to the global loanPool.reward.collateralRewardIndex value

  3. Withdraw all funds from pool

  4. Delete loan to free up loanId, but local index for loanId and poolId persists

  5. User creates loan with loanId for loanTypeId corresponding to smaller global index for different loanPool

  6. User attempts to deposit into loanPool, but tx reverts due to underflow when updating user's collateral rewards, since local_index > global_index

Conditions: The user's createLoan transaction has to specify a loanTypeId that results in a loanPool (when taking into account poolIds) which has a smaller collateralRewardIndex compared to other loanPools for other loanTypeIds (used by bad actor).

Bug Description

A loan is identified by a loanId and holds the following data:

LoanManagerState.sol

65:    struct UserLoanCollateral {
66:        uint256 balance; // denominated in f token
67:        uint256 rewardIndex; // @audit: local index
68:    }
...
85:    struct UserLoan {
86:        bool isActive;
87:        bytes32 accountId;
88:        uint16 loanTypeId;
89:        uint8[] colPools;
90:        uint8[] borPools;
91:        mapping(uint8 poolId => UserLoanCollateral) collaterals;
92:        mapping(uint8 poolId => UserLoanBorrow) borrows;
93:    }

When a loan is deleted, it is first verified that the loan is empty but only the collateral and borrow positions arrays are validated to be empty (see line 66 below). Additionally, the delete keyword is used to reset all of the values in the UserLoan struct for this loanId (see line 69 below). However, the delete keyword does not have an effect on mappings and therefore its possible that the collaterals mapping for a loan will still hold UserLoanCollateral values that were previously stored for a specified poolId key, even after the loan is deleted.

LoanManager::deleteUserLoan

60:    function deleteUserLoan(bytes32 loanId, bytes32 accountId) external override onlyRole(HUB_ROLE) nonReentrant {
61:        // check user loan active and account owner
62:        if (!isUserLoanActive(loanId)) revert UnknownUserLoan(loanId);
63:        if (!isUserLoanOwner(loanId, accountId)) revert NotAccountOwner(loanId, accountId);
64:
65:        // ensure loan is empty
66:        if (!_isUserLoanEmpty(loanId)) revert LoanNotEmpty(loanId); // @audit: considered empty if positions arrays are empty, other dynamic types not checked
67:
68:        // delete by setting isActive to false
69:        delete _userLoans[loanId]; // delete has no effect on mappings: https://docs.soliditylang.org/en/v0.8.26/types.html#delete

LoanManagerState::_isUserLoanEmpty

453:    function _isUserLoanEmpty(bytes32 loanId) internal view returns (bool) {
454:        return _userLoans[loanId].colPools.length == 0 && _userLoans[loanId].borPools.length == 0;

To understand why this can be an issue, we will observe the flow for deposit transaction:

LoanManagerLogic::executeDeposit

66:    function executeDeposit(
67:        mapping(bytes32 => LoanManagerState.UserLoan) storage userLoans,
68:        mapping(uint16 loanTypeId => LoanManagerState.LoanType) storage loanTypes,
69:        mapping(uint8 => IHubPool) storage pools,
70:        mapping(bytes32 accountId => mapping(uint8 poolId => LoanManagerState.UserPoolRewards)) storage userPoolRewards,
71:        DataTypes.ExecuteDepositParams memory params
72:    ) external {
73:        LoanManagerState.UserLoan storage userLoan = userLoans[params.loanId]; // @audit: defined by loanId
74:        LoanManagerState.LoanType storage loanType = loanTypes[userLoan.loanTypeId];
75:        LoanManagerState.LoanPool storage loanPool = loanType.pools[params.poolId]; // @audit: defined by (loanType, poolId)
...
97:        RewardLogic.updateUserCollateralReward(userPoolRewards, userLoan, loanPool, params.poolId); // @audit: update user rewards based on indexes

RewardLogic::updateUserCollateralReward

20:    function updateUserCollateralReward(
21:        mapping(bytes32 accountId => mapping(uint8 poolId => LoanManagerState.UserPoolRewards)) storage userPoolRewards,
22:        LoanManagerState.UserLoan storage loan,
23:        LoanManagerState.LoanPool storage loanPool,
24:        uint8 poolId
25:    ) internal {
26:        LoanManagerState.UserPoolRewards storage userLoanPoolRewards = userPoolRewards[loan.accountId][poolId];
27:        LoanManagerState.UserLoanCollateral storage userLoanCollateral = loan.collaterals[poolId];
28:        uint256 collateralRewardIndex = loanPool.reward.collateralRewardIndex; // @audit: global index 
29:
30:        userLoanPoolRewards.collateral += MathUtils.calcAccruedRewards(
31:            userLoanCollateral.balance,
32:            collateralRewardIndex, // @audit: global index for loanPool, defined by (loanTypeId, poolId)
33:            userLoanCollateral.rewardIndex // @audit: local index for user's UserLoanCollateral, defined by poolId
34:        );
35:
36:        userLoanCollateral.rewardIndex = collateralRewardIndex;

MathUtils::calcAccruedRewards

556:    function calcAccruedRewards(
557:        uint256 amount,
558:        uint256 rewardIndexAtT,
559:        uint256 rewardIndexAtT_1
560:    ) internal pure returns (uint256) {
561:        return Math.mulDiv(amount, rewardIndexAtT - rewardIndexAtT_1, MathUtils.ONE_18_DP); // @audit: global_index - local_index

As we can see above, when updating the user's collateral rewards during deposits, the global index for a loanPool is defined by a loanTypeId and poolId pair. I.e. loanPool = (1, 128) will have a different global index compared to loanPool = (2, 128). However, the local index for the user is defined only by the poolId. Therefore, this local index can be implicitly pointing to a loanPool with loanTypeId of 1 or 2. It is assumed that the local index will be consistent with the UserLoan.loanTypeId value that is specified for the created loan (and therefore consistent with the global index for a loanTypeId, poolId pair). However, we have already seen that the local index for a loan can persist after deletion. Therefore, the following situation can arise:

  • global index for loanPool = (1, 128) is x

  • global index for loanPool = (2, 128) is x + 1

  • Loan is created with loanId and loanTypeId = 2.

  • poolId = 128 is deposited into, setting the loan's local index for poolId = 128 to x + 1

  • loan is emptied (all collateral withdrawn) and deleted

  • New loan is created with loanId and loanTypeId = 1

  • Current local index for loan and poolId = 128 is x + 1, but global index for (loanTypeId, poolId) is x

After the above situation occurs, when the user attempts to deposit into poolId = 128 with the new loan, the transaction will revert on line 561 in MathUtils.sol. This is due to the fact that the global index for the loanPool = (1, 128) is x, which is less than the local index for the new loan and poolId (x + 1). Thus, the rewardIndexAtT - rewardIndexAtT_1 operation will result in an underflow.

Impact

A bad actor is able to block users from depositing into certain pools. This exploit can only occur against users who are creating loans with loanTypeIds that correspon