Closed sherlock-admin2 closed 1 year ago
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
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.
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.
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.
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
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.
Market
only calls request
in its update
function.
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.
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.
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-L124The only callers authorized to
request
from oracle are supposed to beMarket
instances orOracle
instances (when forwardingrequest
to provider). Ultimately the originator of the call is stillMarket
instance.
Market
only callsrequest
in itsupdate
function.There is no function in
Market
to change oracle, it is set in initialization and can't be changedNow, 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 theMarket
instance which makes it behave maliciously (callrequest
and not pay for it for example). If the proposed functionality is implemented, this will makeMarket.update
function simply revert for lack of authorization. However, exactly the same can be achieved by pausingMarket
(as it'sPausable
), which will make it impossible for theMarket
to calloracle.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 ofMarket
calling it, so it's 1-to-1. IfMarket
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.
@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.
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).
In addtion to panprog's comment on pausing malicious Market, we can see factories[factory]
is referred in 3 places:
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.
@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.
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
@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.
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.
https://github.com/sherlock-audit/2023-02-telcoin-judging/issues/67
https://github.com/sherlock-audit/2023-03-teller-judging/issues/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!
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.
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.