slowfi - Potential Loss of Fees Due to Missing Registered Fee Collector in `openPosition` Function

[sherlock-admin3]

slowfi

Medium

Potential Loss of Fees Due to Missing Registered Fee Collector in openPosition Function

Summary

In the openPosition function of PartyBFacetImpl.sol, even if there is a valid affiliate, it is possible that there is no registered fee collector, which may result in storing the fees on the zero address. This issue arises due to the lack of checks for the existence of a registered fee collector for the affiliate address. The relevant code can be found in PartyBFacetImpl.sol lines 97-103.

Vulnerability Detail

The flow for interaction between parties require: PartyA creates a quote, then PartyB can open a position based on that quote previously created. The function for creating a quote by PartyA should be sendQuoteWithAffiliate from the PartyAFacet.sol contract. The function to open a position by PartyB should be openPosition from the PartyBFacet.sol.

Quotes can have affiliates that may receive the balances of the fees on a given interaction. Fees balances are stored on the account storage corresponding to the address stored on the mapping affiliateFeeCollector stored on the global app storage accountLayout.balances[GlobalAppStorage.layout().affiliateFeeCollector[quote.affiliate]].

However there is no check/guarantee that a valid affiliate does have a valid affiliateFeeCollector address assigned. The system does not enforce this requirement. So, it is possible that fees can be stored on the account storage corresponding to address zero.

It is important to remark that this can also happen for quotes created before the upgrade of the code. If positions are opened after as affiliate may be zero fees would also be stored on the address zero account storage.

Impact

Improper handling of funds for the actors of the protocol.

Code Snippet

PartyBFacetImpl.sol#L97-L103

if (quote.orderType == OrderType.LIMIT) {
    require(quote.quantity >= filledAmount && filledAmount > 0, "PartyBFacet: Invalid filledAmount");
    accountLayout.balances[GlobalAppStorage.layout().affiliateFeeCollector[quote.affiliate]] += (filledAmount * quote.requestedOpenPrice * quote.tradingFee) / 1e36;
} else {
    require(quote.quantity == filledAmount, "PartyBFacet: Invalid filledAmount");
    accountLayout.balances[GlobalAppStorage.layout().affiliateFeeCollector[quote.affiliate]] += (filledAmount * quote.marketPrice * quote.tradingFee) / 1e36;
}

Code for creating a quote that does not check for a valid registered fee collector contract. PartyAFacetImpl.sol#L72

require(maLayout.affiliateStatus[affiliate], "PartyAFacet: Invalid affiliate");

// lock funds the in middle of way
accountLayout.pendingLockedBalances[msg.sender].add(lockedValues);
currentId = ++quoteLayout.lastId;

// create quote.
Quote memory quote = Quote({
    id: currentId,
    partyBsWhiteList: partyBsWhiteList,
    symbolId: symbolId,
    positionType: positionType,
    orderType: orderType,
    openedPrice: 0,
    initialOpenedPrice: 0,
    requestedOpenPrice: price,
    marketPrice: upnlSig.price,
    quantity: quantity,
    closedAmount: 0,
    lockedValues: lockedValues,
    initialLockedValues: lockedValues,
    maxFundingRate: maxFundingRate,
    partyA: msg.sender,
    partyB: address(0),
    quoteStatus: QuoteStatus.PENDING,
    avgClosedPrice: 0,
    requestedClosePrice: 0,
    parentId: 0,
    createTimestamp: block.timestamp,
    statusModifyTimestamp: block.timestamp,
    quantityToClose: 0,
    lastFundingPaymentTimestamp: 0,
    deadline: deadline,
    tradingFee: symbolLayout.symbols[symbolId].tradingFee,
    affiliate: affiliate
});
quoteLayout.quoteIdsOf[msg.sender].push(currentId);
quoteLayout.partyAPendingQuotes[msg.sender].push(currentId);
quoteLayout.quotes[currentId] = quote;

uint256 fee = LibQuote.getTradingFee(currentId);
accountLayout.allocatedBalances[msg.sender] -= fee;
emit SharedEvents.BalanceChangePartyA(msg.sender, fee, SharedEvents.BalanceChangeType.PLATFORM_FEE_OUT);

Code for creating a valid affiliate ControlFacet.sol#L80-L84

function registerAffiliate(address affiliate) external onlyRole(LibAccessibility.AFFILIATE_MANAGER_ROLE) {
    require(!MAStorage.layout().affiliateStatus[affiliate], "ControlFacet: Address is already registered");
    MAStorage.layout().affiliateStatus[affiliate] = true;
    emit RegisterAffiliate(affiliate);
}

Code for registering a fee collector ControlFacet.sol#L141-L146

function setFeeCollector(address affiliate, address feeCollector) external onlyRole(LibAccessibility.AFFILIATE_MANAGER_ROLE) {
    require(feeCollector != address(0), "ControlFacet: Zero address");
    require(MAStorage.layout().affiliateStatus[affiliate], "ControlFacet: Invalid affiliate");
    emit SetFeeCollector(affiliate, GlobalAppStorage.layout().affiliateFeeCollector[affiliate], feeCollector);
    GlobalAppStorage.layout().affiliateFeeCollector[affiliate] = feeCollector;
}

Tool used

Manual Review

Recommendation

Add checks to verify that GlobalAppStorage.layout().affiliateFeeCollector[quote.affiliate] is not the zero address. Consider performing this check when the quote is created. Also consider creating a workflow for quotes created before the update that may be opened after the code is upgraded. Other possible approach is to ensure to enforce registering a fee collector when registering the affiliate.

[sherlock-admin4]

2 comment(s) were left on this issue during the judging contest.

PNS commented:

dup: #021 - the same conceptual error, affiliate is not checked whether it is valid

Hash01011122 commented:

Invalid

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits: https://github.com/SYMM-IO/protocol-core/pull/53

[slowfinance]

Escalate

This issue is not a dup of #21. Issue 21 illustrates how the sendQuote function sets address(0) on the affiliate and the consequences it may have.

This issue illustrate how fees can be stored on the zero address as the protocol does not enforce to have a valid fee collector for an already registered affiliate. This allows the contract to move to a state of loss of user funds by storing fees on the address(0) storage balance.

This issue is valid according to Sherlock’s judging rules (https://docs.sherlock.xyz/audits/judging/judging#v.-how-to-identify-a-medium-issue)

V. How to identify a medium issue: Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

[sherlock-admin3]

Escalate

This issue is not a dup of #21. Issue 21 illustrates how the sendQuote function sets address(0) on the affiliate and the consequences it may have.

This issue illustrate how fees can be stored on the zero address as the protocol does not enforce to have a valid fee collector for an already registered affiliate. This allows the contract to move to a state of loss of user funds by storing fees on the address(0) storage balance.

This issue is valid according to Sherlock’s judging rules (https://docs.sherlock.xyz/audits/judging/judging#v.-how-to-identify-a-medium-issue)

V. How to identify a medium issue: Causes a loss of funds but requires certain external conditions or specific states, or a loss is highly constrained. The losses must exceed small, finite amount of funds, and any amount relevant based on the precision or significance of the loss.

The escalation could not be created because you are not exceeding the escalation threshold.

You can view the required number of additional valid issues/judging contest payouts in your Profile page, in the Sherlock webapp.

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.