FundsHandlerFacet Code Style Findings

FHF-01C: Inefficient mapping Lookups

Type	Severity	Location
Gas Optimization		FundsHandlerFacet.sol:L174, L225

Description:

The linked statements perform key-based lookup operations on mapping declarations from storage multiple times for the same key redundantly.

Example:

Copy
223for (uint256 i = 0; i < len; i++) {
224    // get available funds from storage
225    uint256 availableFunds = protocolLookups().availableFunds[_entityId][tokenList[i]];
226    FundsLib.transferFundsFromProtocol(_entityId, tokenList[i], _destinationAddress, availableFunds);
227}

Recommendation:

As the lookups internally perform an expensive keccak256 operation, we advise the lookups to be cached wherever possible to a single local declaration that either holds the value of the mapping in case of primitive types or holds a storage pointer to the struct contained.

Alleviation (44009967e4f68092941d841e9e0f5dd2bb31bf0b):

The referenced mapping lookups have been optimized to the greatest extent possible thus greatly reducing the gas cost of the codebase.

FHF-02C: Repetitive Invocations of Getter Functions

Type	Severity	Location
Gas Optimization		FundsHandlerFacet.sol:L94, L97, L100, L106, L109, L154, L174, L215, L225

Description:

The linked statements repetitively invoke the same getter functions whilst their return values remain unchanged.

Example:

Copy
85function withdrawFunds(
86    uint256 _entityId,
87    address[] calldata _tokenList,
88    uint256[] calldata _tokenAmounts
89) external override fundsNotPaused nonReentrant {
90    // address that will receive the funds
91    address payable destinationAddress;
92
93    // first check if the caller is a buyer
94    (bool exists, uint256 callerId) = getBuyerIdByWallet(msgSender());
95    if (exists && callerId == _entityId) {
96        // caller is a buyer
97        destinationAddress = payable(msgSender());
98    } else {
99        // check if the caller is a clerk
100        (exists, callerId) = getSellerIdByClerk(msgSender());
101        if (exists && callerId == _entityId) {
102            // caller is a clerk. In this case funds are transferred to the treasury address
103            (, Seller storage seller, ) = fetchSeller(callerId);
104            destinationAddress = seller.treasury;
105        } else {
106            (exists, callerId) = getAgentIdByWallet(msgSender());
107            if (exists && callerId == _entityId) {
108                // caller is an agent
109                destinationAddress = payable(msgSender());
110            } else {
111                // in this branch, caller is neither buyer, clerk or agent or does not match the _entityId
112                revert(NOT_AUTHORIZED);
113            }
114        }
115    }
116
117    withdrawFundsInternal(destinationAddress, _entityId, _tokenList, _tokenAmounts);
118}

Recommendation:

We advise the getter functions to be invoked once and their results to be stored to local variables that are consequently utilized optimizing the gas cost of the statements significantly.

Alleviation (44009967e4f68092941d841e9e0f5dd2bb31bf0b):

All repetitive getter function invocations have been reduced to a single getter invocation per repetitive instance thus optimizing the codebase significantly.

FHF-03C: Suboptimal Struct Declaration Style

Type	Severity	Location
Code Style		FundsHandlerFacet.sol:L177

Description:

The linked declaration style of a struct is using index-based argument initialization.

Example:

Copy
177availableFunds[i] = Funds(tokenAddress, tokenName, availableAmount);

Recommendation:

We advise the key-value declaration format to be utilized instead, greatly increasing the legibility of the codebase.

Alleviation (44009967e4):

The Funds struct creation has instead been adjusted to three separate assignment statements which we consider sub-optimal. We advised the usage of the key-value declaration style (i.e. Funds(a, b, c) would become Funds({tokenAddress:a,tokenName:b,availableAmount:c})) to optimize the code's legibility.

Alleviation (6dae5d2602):

The Boson Protocol team stated that they wish to retain the newly introduced per-key assignment format to ensure consistency across the codebase and to also permit instantiation of structs with members that are otherwise impossible to copy by memory (i.e. in-memory arrays). Given that the usage of the per-key style is a valid case for types that cannot be assigned in a direct struct instantiation statement, we consider this exhibit adequately dealt with as the ambiguity portion of the exhibit has been sufficiently dealt with.