sherlock-admin
commented
11 months ago 3 comment(s) were left on this issue during the judging contest.
panprog commented:
invalid because it's impossible to set ready manually, it's a function!
n33k commented:
invalid, not convincing without PoC
darkart commented:
No explanation in how it would manipulate it ?
feelereth
high
Vulnerability where an attacker could manipulate the _mappings storage to make it appear that the next global checkpoint is ready when it actually is not.
Summary
When settling, it assumes _mappings[context.global.latest + 1].read().ready(context.latestIds) means the global state is ready to move to the next checkpoint. But an attacker could manipulate mappings to make it appear ready when it's not.
Vulnerability Detail
The relevant code is in the _settle function: link The _mappings[id].ready(ids) check is intended to ensure all markets have reached the next checkpoint id before advancing the global checkpoint. However, an attacker could manipulate _mappings directly to set _mappings[context.global.latest + 1].ready to true even when markets are not actually ready. This would cause _settle to incorrectly advance the global checkpoint prematurely.
Impact
• Incorrect global state if markets are settled at different points • Potential loss of funds if users deposit/redeem based on invalid global state
Code Snippet
https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/Vault.sol#L346-L348
Tool used
Manual Review
Recommendation
_mappings[id].ready should be replaced with a more robust check that cannot be manipulated, for example:
// check each market directly for (uint i = 0; i < totalMarkets; i++) { if (_registrations[i].currentId <= context.global.latest) { // market not ready yet return; } } // all markets ready, advance global ...
This verifies each market's checkpoint directly rather than relying on the potentially manipulated _mappings. Additional checks could also be added, like verifying the total collateral in _checkpoints[context.global.latest+1] matches the sum from markets before advancing. This would provide certainty the global state is valid.