search

sherlock-audit / 2023-09-perennial-judging

0 stars 0 forks source link

feelereth - Collateral values from different markets are denominated in the same units, which can lead to issues summing them. #47

Closed sherlock-admin2 closed 1 year ago

sherlock-admin2 commented 1 year ago

feelereth

high

Collateral values from different markets are denominated in the same units, which can lead to issues summing them.

Summary

The assumption of common collateral units is dangerous and could be manipulated. Collateral should be normalized before summing across markets

Vulnerability Detail

The key code sections are:

  1. _collateral function: Link 1. This sums the collateral from the asset balance and each market into a single value.
  2. _collateralAtId function: Link 2 . Similar logic alos, summing the collateral from each market's position into a single value.

Impact

If markets track collateral in different units (e.g. one in ETH, another in DAI), summing them doesn't make sense and could lead to incorrect totals. For example: • Market 1 collateral: 10 ETH • Market 2 collateral: 1000 DAI • Summed collateral would be 1010, which is incorrect. This could allow attackers to deposit "fake" collateral that looks larger when summed.

Code Snippet

https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/Vault.sol#L569-L573 https://github.com/sherlock-audit/2023-09-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/Vault.sol#L581-L589

Tool used

Manual Review

Recommendation

Markets should standardize collateral units, or collateral should be converted to common units before summing

sherlock-admin commented 1 year ago

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

panprog commented:

invalid because all markets use DSU token as collateral

n33k commented:

invalid, not convincing without PoC