Emmanuel - Protocol fee from Market.sol is locked

[sherlock-admin]

Emmanuel

high

Protocol fee from Market.sol is locked

Summary

The MarketFactory#fund calls the specified market's Market#claimFee function. This will send the protocolFee to the MarketFactory contract. MarketFactory contract does not max approve any address to spend its tokens, and there is no function that can be used to get the funds out of the contract, so the funds are permanently locked in MarketFactory.

Vulnerability Detail

Here is MarketFactory#fund function:

  function fund(IMarket market) external {
        if (!instances(IInstance(address(market)))) revert FactoryNotInstanceError();
@>      market.claimFee();
    }

This is Market#claimFee function:

    function claimFee() external {
        Global memory newGlobal = _global.read();

        if (_claimFee(address(factory()), newGlobal.protocolFee)) newGlobal.protocolFee = UFixed6Lib.ZERO;
        ...
    }

This is the internal _claimFee function:

    function _claimFee(address receiver, UFixed6 fee) private returns (bool) {
        if (msg.sender != receiver) return false;

        token.push(receiver, UFixed18Lib.from(fee));
        emit FeeClaimed(receiver, fee);
        return true;
    }

As we can see, when MarketFactory#fund is called, Market#claimFee gets called which will send the protocolFee to msg.sender(MarketFacttory). When you check through the MarketFactory contract, there is no place where another address(such as protocol multisig, treasury or an EOA) is approved to spend MarketFactory's funds, and also, there is no function in the contract that can be used to transfer MarketFactory's funds. This causes locking of the protocol fees.

Impact

Protocol fees cannot be withdrawn

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/MarketFactory.sol#L89

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L133

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial/contracts/Market.sol#L145-L151

Tool used

Manual Review

Recommendation

Consider adding a withdraw function that protocol can use to get the protocolFee out of the contract. You can have the withdraw function transfer the MarketFactory balance to the treasury or something.

[sherlock-admin]

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

141345 commented:

h

n33k commented:

medium

[arjun-io]

We originally wanted to keep the funds in the Factory (for a future upgrade) but it might make sense to instead allow the Factory Owner (Timelock) to claim these funds instead

[Emedudu]

Escalate

I believe this is of HIGH severity because funds are permanently locked

[sherlock-admin2]

Escalate

I believe this is of HIGH severity because funds are permanently locked

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[arjun-io]

Fixed https://github.com/equilibria-xyz/perennial-v2/pull/79

re: Escalation - since this contract can be upgraded the funds are not permanently locked

[Emedudu]

Fixed https://github.com/equilibria-xyz/perennial-v2/pull/79

Can't access the repo to review the fix. It's probably a private repo.

since this contract can be upgraded the funds are not permanently locked

While it's true that the contract can potentially be upgraded to address this issue, it's essential to acknowledge that the current code we audited does, in fact, contain a high severity vulnerability. Otherwise, implying that all upgradeable contracts are free of bugs simply because they can be upgraded to resolve them would be misleading.

[arjun-io]

Otherwise, implying that all upgradeable contracts are free of bugs simply because they can be upgraded to resolve them would be misleading.

The distinction here is that "funds stuck" are fixable via upgrades, whereas attacks which immediately drain funds or those which cause accounting errors are not after they are executed.

[hrishibhat]

Result: High Has duplicates Although Sponsor raises a valid point, perhaps Sherlock needs to have a rule with respect to smart contract upgrade-related issues. Historically smart contract upgrades have not weighed on the issue severity decisions but will be considered in future rule updates, however, this issue will be considered as a valid high issue based on historical decisions on similar issues.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• Emedudu: accepted

[jacksanford1]

From WatchPug:

Fixed