ak1 - OracleFactory.sol : No way to unregister the factory and remove the authorization of a caller in OracleFactory contract.

[sherlock-admin2]

ak1

medium

OracleFactory.sol : No way to unregister the factory and remove the authorization of a caller in OracleFactory contract.

Summary

OracleFactory.sol has the following two functions to register the factory contract and authorize a caller.

    function register(IOracleProviderFactory factory) external onlyOwner {
        factories[factory] = true;
        emit FactoryRegistered(factory);
    }


    /// @notice Authorizes a factory's instances to request from this contract
    /// @param caller The factory to authorize
    function authorize(IFactory caller) external onlyOwner {
        callers[caller] = true;
        emit CallerAuthorized(caller);
    }

But there are not function to revoke the above permission when any of them or both of them turns into malicious or malfunctional.

Vulnerability Detail

Refer the summary section

Impact

The permission never be revoked when any of them or both of them turns into malicious or malfunctional.
Contract would suffer with these malicious or malfunctional factory and caller.

Code Snippet

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-oracle/contracts/OracleFactory.sol#L41-L51

Tool used

Manual Review

Recommendation

Instead of setting true in both of the functions, use a bool flag to update the permission.
This will give control over these two elements.

[sherlock-admin]

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

141345 commented:

o

panprog commented:

low (invalid) because owner is trusted and caller is supposed to be only the Market contract which can't become malicious

[aktech297]

Escalate

This issue is not talking about input setting by the owner.

Its explains about the nature of implementation where owner can never have control over the oracle factory provider and the Factory contract.

when they are compromised or turns into malicious owner can never reset them to not use.

These type of issues are treated as medium in sherlock judging historically.

[sherlock-admin2]

Escalate

This issue is not talking about input setting by the owner.

Its explains about the nature of implementation where owner can never have control over the oracle factory provider and the Factory contract.

when they are compromised or turns into malicious owner can never reset them to not use.

These type of issues are treated as medium in sherlock judging historically.

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.

[panprog]

Escalate

While I agree that this functionality is good to have, however I only see of this as low, not medium, due to impact. Impact stated in the report is:

The permission never be revoked when any of them or both of them turns into malicious or malfunctional.
Contract would suffer with these malicious or malfunctional factory and caller.

It's very vague. Remember, the caller is authorized by trusted admin, so he is supposed to only do what is expected, if he does something not expected from the usage, this is invalid since he is trusted.
Let's see what can actually happen if admin behaves the way he is supposed.

1. The only action available to authorized addresses is request:
   https://github.com/sherlock-audit/2023-07-perennial/blob/120bcfef4028654de83477ffe992805ddada1043/perennial-v2/packages/perennial-oracle/contracts/Oracle.sol#L35
   https://github.com/sherlock-audit/2023-07-perennial/blob/120bcfef4028654de83477ffe992805ddada1043/perennial-v2/packages/perennial-oracle/contracts/Oracle.sol#L120-L124

2. The only callers authorized to request from oracle are supposed to be Market instances or Oracle instances (when forwarding request to provider). Ultimately the originator of the call is still Market instance.

3. Market only calls request in its update function.

4. There is no function in Market to change oracle, it is set in initialization and can't be changed

Now, since the only authorized call for the oracle is in Market.update, Market has to become malicious somehow. Let's suppose there is some bug in the Market instance which makes it behave maliciously (call request and not pay for it for example). If the proposed functionality is implemented, this will make Market.update function simply revert for lack of authorization. However, exactly the same can be achieved by pausing Market (as it's Pausable), which will make it impossible for the Market to call oracle.request.

However, I also want to add that while I don't fully understand the exact use case for oracle instance, but I suppose that each Oracle instance will have only 1 instance of Market calling it, so it's 1-to-1. If Market becomes malicious somehow, Oracle instance will also be useless anyway, so there is no impact in the first place, simply because malicious caller automatically renders oracle useless.

[sherlock-admin2]

Escalate

While I agree that this functionality is good to have, however I only see of this as low, not medium, due to impact. Impact stated in the report is:

The permission never be revoked when any of them or both of them turns into malicious or malfunctional.
Contract would suffer with these malicious or malfunctional factory and caller.

It's very vague. Remember, the caller is authorized by trusted admin, so he is supposed to only do what is expected, if he does something not expected from the usage, this is invalid since he is trusted.
Let's see what can actually happen if admin behaves the way he is supposed.

1. The only action available to authorized addresses is request:
   https://github.com/sherlock-audit/2023-07-perennial/blob/120bcfef4028654de83477ffe992805ddada1043/perennial-v2/packages/perennial-oracle/contracts/Oracle.sol#L35
   https://github.com/sherlock-audit/2023-07-perennial/blob/120bcfef4028654de83477ffe992805ddada1043/perennial-v2/packages/perennial-oracle/contracts/Oracle.sol#L120-L124

2. The only callers authorized to request from oracle are supposed to be Market instances or Oracle instances (when forwarding request to provider). Ultimately the originator of the call is still Market instance.

3. Market only calls request in its update function.

4. There is no function in Market to change oracle, it is set in initialization and can't be changed

Now, since the only authorized call for the oracle is in Market.update, Market has to become malicious somehow. Let's suppose there is some bug in the Market instance which makes it behave maliciously (call request and not pay for it for example). If the proposed functionality is implemented, this will make Market.update function simply revert for lack of authorization. However, exactly the same can be achieved by pausing Market (as it's Pausable), which will make it impossible for the Market to call oracle.request.

However, I also want to add that while I don't fully understand the exact use case for oracle instance, but I suppose that each Oracle instance will have only 1 instance of Market calling it, so it's 1-to-1. If Market becomes malicious somehow, Oracle instance will also be useless anyway, so there is no impact in the first place, simply because malicious caller automatically renders oracle useless.

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.

[hrishibhat]

@panprog @141345
In addition to panprog's points,
If I understand that correctly there is no need to unregister since the new factory can be updated into that respective oracle here right?
If that's true don't see an issue here.

[panprog]

If I understand that correctly there is no need to unregister since the new factory can be updated into that respective oracle here right?

@hrishibhat I think update can be used to set a new provider and if no authorization is given for new provider, oracle itself will revert for lack of authorization. The flow is the following:

Market -> oracle.request() -> provider1.request()

So Market is authorized by oracle, oracle is authorized by provider1

If update(provider2) is called, then the flow becomes

Market -> oracle.request() -> provider2.request()

Market is still authorized by oracle, however if provider2 doesn't authorize oracle, then the whole call flow will revert. So this is another way to prevent malicious market from calling authorized function, however if there are the other callers authorized by oracle, their calls will also revert, so I think a better way to prevent unauthorized calls is pausing malicious Market, this will allow oracle to keep receiving requests correctly from all the other authorized callers (if any).

[141345]

In addtion to panprog's comment on pausing malicious Market, we can see factories[factory] is referred in 3 places:

• line 57: create()
• line 73: update()
• line 93: claim()

create() and update() are both admin functions, so there won't be problem. claim() is one time for existing rewards.

So even if facotry turned malicious, it still needs malicious admin to cause further loss.

[aktech297]

@hrishibhat Adding points to this issue

For factory provider, as mentioned by @141345 claim would be the cause of concern. Here, the factory is allowed to claim the incentive for their work which they do.

But, the factory is malicious or compromised as mentioned in my report. They still can claim the incentive. (Note, even if the owner want to stop, they can not do it. because owner can not unregister this factory address)

This would lead to loss of asset to the protocol.

Also, the same factory can claim again and again. This will lead to situation where other factories can not be able to claim their incentive. This will disincentive the other factory address so they will lose interest on the work which they do.

On the callers where the factory instance is authorized, this will be used in Oracle contract for request.

    function request(address account) external onlyAuthorized {
        (OracleVersion memory latestVersion, uint256 currentTimestamp) = oracles[global.current].provider.status();

        oracles[global.current].provider.request(account); --------------------->> update the provide address.
        oracles[global.current].timestamp = uint96(currentTimestamp);
        _updateLatest(latestVersion); -------------------------------------->> sets the latest version.
    }

As we can see that the request is called only the Authorized caller with input account. This account would be set as provider. Again, setting the account would be a cause of concern, where the compromised or malicious caller can set their own address or any other contract address which can provide incorrect data to the market.

By looking at above points, this issue could be more serious one. imo, I would look for High.

[hrishibhat]

Result:
Low
Unique
These permissions are granted by a trusted role and after discussing this with the Sponsor this is not an issue based on the trust assumptions mentioned.
Considering this a valid low

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• panprog: accepted
• aktech297: rejected

[aktech297]

@hrishibhat you need to check the past judging history to know how this issue is treated. It's been there as valid medium. I clearly see that Sherlock doesn't have consistent metric to judge the issues even if sponser disagree.

[aktech297]

Adding @jacksanford1 for more opinion on this.

requesting to re-consider the escalation for consistent judging by following the sherlock rules.

Hi @hrishibhat, below are the couple of issues where Sherlock judges them valid high.

sherlock-audit/2023-02-telcoin-judging#67

sherlock-audit/2023-03-teller-judging#339

There are couple more issues as well from other contest. As per sherlock judging guidelines, even if it is set by admin , but after setting, the control go out of admin , these issues are treated as valid.

I hope that sherlock would give consistent judging here.

Thanks!