Boost _ Folks Finance 33779 - [Smart Contract - Medium] The account creation process can be griefed

Submitted on Mon Jul 29 2024 08:49:26 GMT-0400 (Atlantic Standard Time) by @JCN2023 for Boost | Folks Finance

Report ID: #33779

Report type: Smart Contract

Report severity: Medium

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

Impacts:

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

Description

Brief/Intro

An account is required to perform any core operations in the folks finance protocol. It is the user's responsibility to choose their accountId that will be created. A bad actor can front-run any account creation operation on the Hub chain and create an account using the accountId seen in any user's tx. Therefore, the bad actor is able to grief the account creation of any other user, preventing them from being able to interact with the protocol for an arbitrary amount of time.

Bug Description

Ignoring cross-chain components, the execution flow for account creation is as follows: SpokeCommon::createAccount -> router/adapter/hub interactions -> AccountManager::createAccount. It is important to note that all the account state is stored on the Hub chain, so the AccountManager contract on the Hub chain will store the account information for all users, even if the user is interacting via a different Spoke chain.

When creating an account, a user must supply an accountId that will be the identifier for the account to be created:

SpokeCommon::createAccount

27:    function createAccount(
28:        Messages.MessageParams memory params,
29:        bytes32 accountId, // @audit: user defined
30:        bytes32 refAccountId
31:    ) external payable nonReentrant {
32:        _doOperation(params, Messages.Action.CreateAccount, accountId, abi.encodePacked(refAccountId));
33:    }

The accountId is then validated to be non zero and must not have already been created:

AccountManager::createAccount

35:    function createAccount(
36:        bytes32 accountId,
37:        uint16 chainId,
38:        bytes32 addr,
39:        bytes32 refAccountId
40:    ) external override onlyRole(HUB_ROLE) {
41:        // check account is not already created (empty is reserved for admin)
42:        if (isAccountCreated(accountId) || accountId == bytes32(0)) revert AccountAlreadyCreated(accountId); // @audit: accountId should not be created already
43:
44:        // check address is not already registered
45:        if (isAddressRegistered(chainId, addr)) revert AddressPreviouslyRegistered(chainId, addr);
46:
47:        // check referrer is well defined
48:        if (!(isAccountCreated(refAccountId) || refAccountId == bytes32(0)))
49:            revert InvalidReferrerAccount(refAccountId);
50:
51:        // create account
52:        accounts[accountId] = true;

AccountManager::isAccountCreated

217:    function isAccountCreated(bytes32 accountId) public view override returns (bool) {
218:        return accounts[accountId]; // @audit: value is true if already created
219:    }

Since this accountId value is user defined, any bad actor can front-run other users' createAccount transactions and create accounts using those users' accountIds. This will result in the users' transactions reverting on line 42 in AccountManager.sol.

Impact

A bad actor can consistently grief other users' createAccount transactions, preventing those users from interacting with the core components of the protocol for an arbitrary amount of time. The bad actor can lower the gas costs of this exploit by submitting their createAccount tx directly via the Hub chain (utilizing the HubAdapter) instead of interacting via a Spoke chain (triggering cross chain communications).

Recommended Mitigation

I would recommend implementing an accountId state variable that is utilized and incremented every time a new account is created. This would be more gas intensive, but would limit the number of arbitrary values that users can supply to functions, which will in turn decrease the number of possible attack vectors.

Proof of concept

Proof of Concept

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_GriefAccountCreation

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

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

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

interface IAccountManager {
    function isAccountCreated(bytes32 accountId) external view returns (bool);
    function isAddressRegisteredToAccount(bytes32 account, uint16 chainId, bytes32 addr) external view returns (bool);
}

interface ISpoke {
    struct MessageParams {
        uint16 adapterId; 
        uint16 returnAdapterId; 
        uint256 receiverValue; 
        uint256 gasLimit; 
        uint256 returnGasLimit; 
    }

    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;
}

interface ILoanManager {
    function isUserLoanActive(bytes32 loanId) external view returns (bool);
}

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

contract FolksPOC_GriefAccountCreation is Test {
    uint256 avaxTestnetFork;

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

    address constant HUB = 0xaE4C62510F4d930a5C8796dbfB8C4Bc7b9B62140;

    address constant HUB_ADAPTER = 0xf472ab58969709De9FfEFaeFFd24F9e90cf8DbF9;

    address constant SPOKE_COMMON = 0x6628cE08b54e9C8358bE94f716D93AdDcca45b00;

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

    error AccountAlreadyCreated(bytes32 accountId);

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

        vm.selectFork(avaxTestnetFork);
    }

    function testGriefAccountCreation() public {
        // user address
        address user = address(0x1234);

        // bad actor address
        address badActor = address(0x69420);