Closed sherlock-admin4 closed 4 months ago
1 comment(s) were left on this issue during the judging contest.
panprog commented:
low or borderline medium as this seems more an admin parameters choice tweak which should be considered a trusted setup
request poc
This seems invalid, but could be a good improvement. Don't think paying more fees for pyth oracle is a security risk
Sponsor comments:
yeah we use the new VAA format now, there's nothing VAA specific in the code so either is fine with our contracts. Agreed that it's invalid either way
I'm guessing they just assumed we're still using the old format from the tests
PoC requested from @ydspa
Requests remaining: 1
I think the report and the existing PoC has clearly represent what i wanna show:
The protocol is paying about 36%
extra compensation for keepers to call PythFactory.commit()
, but the intended and reasonable compensation rate should be much lower, may be same with call for PythFactory.settle()
, which is only 5%
.
And this issue is existing since Perennial V2's initial deployment: https://arbiscan.io/address/0x6b60e7c96b4d11a63891f249ea826f8a73ef4e6e
@kbrizzle @arjun-io Was this a known issue in previous audit and/or known in contest details?
You can see in this tenderly decoded tx (sent to the protocol recently) that the Pyth update payload is the new 0x504...
format: https://dashboard.tenderly.co/tx/42161/0x5939c26a88820610e10d3a310da810c35dbfa4c8091de821960bf6ce0d78904b
@ydspa Do you agree this issue should be invalid?
You can see in this tenderly decoded tx (sent to the protocol recently) that the Pyth update payload is the new
0x504...
format: https://dashboard.tenderly.co/tx/42161/0x5939c26a88820610e10d3a310da810c35dbfa4c8091de821960bf6ce0d78904b
The issue here is exactly that keepers can commit price with the new VAA format, but Perennial V2 keeps paying fee based the Old VAA, and result in a 36%
extra compensation for keepers.
You can see in this tenderly decoded tx (sent to the protocol recently) that the Pyth update payload is the new
0x504...
format: https://dashboard.tenderly.co/tx/42161/0x5939c26a88820610e10d3a310da810c35dbfa4c8091de821960bf6ce0d78904bThe issue here is exactly that keepers can commit price with the new VAA format, but Perennial V2 keeps paying fee based the Old VAA, and result in a
36%
extra compensation for keepers.
Can you point to the code where we're overpaying based on the old VAA?
You can see in this tenderly decoded tx (sent to the protocol recently) that the Pyth update payload is the new
0x504...
format: https://dashboard.tenderly.co/tx/42161/0x5939c26a88820610e10d3a310da810c35dbfa4c8091de821960bf6ce0d78904bThe issue here is exactly that keepers can commit price with the new VAA format, but Perennial V2 keeps paying fee based the Old VAA, and result in a
36%
extra compensation for keepers.Can you point to the code where we're overpaying based on the old VAA?
6%
extra compensation, which is very close to the compensation rate for settle()
.I think it's reasonable to infer that the current implementation is tuned for old VAA rather than the new one. Or is there any doc or reason why set 36% bonus rate for commit()
but only set 5% for settle()
.
@ydspa Wouldn't this be out of scope if you discovered the issue from a deployment script? Isn't either format both compatible with perennial?
@ydspa Wouldn't this be out of scope if you discovered the issue from a deployment script?
Actually it's an online issue on the live net, im not sure it's in or out of scope according Sherlock rule
@ydspa Isn't either format both compatible with perennial?
The two formats are functionally both working, but the Pyth network doesn't provide the old format any more.
My opinion here is that 5% or 36% is just the admin/protocol chosen value: this is the profit (incentive) for the keepers to do their job. Both 5% and 36% are correct values, it's not a loss of funds, it's a balance between settlement fee charged from users and keepers profits. This is a great informational issue so that admins can make more informed decision on the fee, but certainly not medium.
Agree with comments by @panprog.
My opinion here is that 5% or 36% is just the admin/protocol chosen value: this is the profit (incentive) for the keepers to do their job. Both 5% and 36% are correct values, it's not a loss of funds, it's a balance between settlement fee charged from users and keepers profits. This is a great informational issue so that admins can make more informed decision on the fee, but certainly not medium.
Why 36% is a correct value, its obviously an unintended behavior? Paying more fund than intend is a issue
@ydspa Even if so, I believe you discovered this issue from out of scope deployment files and on-chain contracts correct? Unless there is current in-scope- smart contract logic indicating the old VAA is used, I believe this issue to be informational and out of scope
@ydspa Even if so, I believe you discovered this issue from out of scope deployment files and on-chain contracts correct? Unless there is current in-scope- smart contract logic indicating the old VAA is used, I believe this issue to be informational and out of scope
Yep, we can treat this issue as an in scope contract is affected by some out of scope file, and i think it should apply the Sherlock's similar rule for third party library:
Contract Scope: In case the vulnerability exists in a library and an in-scope contract uses it and is affected by this bug this is a valid issue.
@ydspa
Do I understand it correctly that this issue is for the on-chain settings values (commitConfig
?)? I don't see any issue with the code. Or do you suggest any fixes in the code that can fix this issue?
Any settings (i.e. exact values set), including current live on-chain settings are out of scope. It is assumed that admin sets correct settings since admin is trusted.
Even if you find that live on-chain admin settings are bad and indicate a critical vulnerability which can be used to steal all funds, it's still out of scope of this audit contest. This might be a valid issue for bugs bounty program (since it concerns live deployment), but not sure about it.
Escalate
Bring this issue up, look forward a clear Sherlock rule for cases that If an in scope contract is affected by the deployment script, it's valid or not?
Yep, we can treat this issue as an in scope contract is affected by some out of scope file, and i think it should apply the Sherlock's similar rule for third party library: Contract Scope: In case the vulnerability exists in a library and an in-scope contract uses it and is affected by this bug this is a valid issue.
Escalate
Bring this issue up, look forward a clear Sherlock rule for cases that
If an in scope contract is affected by the deployment script, it's valid or not?
Yep, we can treat this issue as an in scope contract is affected by some out of scope file, and i think it should apply the Sherlock's similar rule for third party library: Contract Scope: In case the vulnerability exists in a library and an in-scope contract uses it and is affected by this bug this is a valid issue.
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.
@ydspa, @Czar102 initial response internally is that he agrees, but yea I think we could wait till a more detailed explanation by him incase I am wrong. This to me is a suboptimal admin configuration that makes it out of scope.
@nevillehuang @ydspa If I'm correct, the core issue is a that the keeper fee is higher than required. And the fix is to lower the keeper fee, through an admin call?
Protocol admin: TRUSTED
Protocol admin is considered to be trusted in most cases, hence issues where Admin incorrectly enters an input parameter. Example: Make sure interestPerMin > 1 ether as it is an important parameter. This is not a valid issue.
If I'm correct. I believe the following rule applies and the escalation should be rejected and the issue should stay invalid
@Evert0x The fix would be to utilize the new VAA format via adjustments in the deployment scripts. This will lower the keeper fee paid out. I wouldn't call it an incorrect input but rather suboptimal because the protocol will function as per normal just with higher keeper fees. But since admins are trusted for such configurations, I believe it is still out of scope.
@nevillehuang @ydspa If I'm correct, the core issue is a that the keeper fee is higher than required. And the fix is to lower the keeper fee, through an admin call?
Protocol admin: TRUSTED
Protocol admin is considered to be trusted in most cases, hence issues where Admin incorrectly enters an input parameter. Example: Make sure interestPerMin > 1 ether as it is an important parameter. This is not a valid issue.
If I'm correct. I believe the following rule applies and the escalation should be rejected and the issue should stay invalid
I agree this issue could be treated as incorrect admin input of immutable
parameter, the fix needs an upgrade of contract, and the different point is its not a hypothesis of things like IF admin inputs invalid parameter...
, but truly exists both online and current deployment script.
Based on the comments above it's clear to me that this issue shouldn't be judges as Medium or High.
Planning to reject escalation and keep issue state as is
Result: Invalid Unique
KingNFT
medium
Pyth
oracle is paying30%
more than intended keep feeSummary
After Pyth network's
perseus
upgrade (Sep-20-2023
), the gas cost of price submission has dramatically decreased. Perennial V2 is paying 30% more than intended keeper fee due to lack of adaptation with the upgrade.Reference: https://pyth.network/blog/perseus-network-upgrade
Vulnerability Detail
A key improvement of
perseus
upgrade is that the VAA length has significantly decreased, it reduces both L1 calldata fee and L2 execution fee. As most cases L1 calldata fee is the dominant part of gas cost for L2 transactions, the following PoC shows the difference between actual calldata gas used and the amount Perennial V2 is paying. We can see the current implementation is tuned for the old VAA format with about6%
extra compensation for keepers. But for the new VAA format, the compensation increases to36%
, there is a30%
over payment than intended.the test log:
Impact
The protocol continuously loses fund.
Code Snippet
https://github.com/sherlock-audit/2024-02-perennial-v2-3/blob/main/root/contracts/attribute/Kept/Kept_Arbitrum.sol#L21
Tool used
Manual Review
Recommendation
Decreasing L1 calldata and L2 execution keeper fee based on the new VAA format.