search

sherlock-audit / 2023-07-perennial-judging

2 stars 1 forks source link

tives - Early Vault depositor can manipulate exchange rates to steal funds from later depositors #154

Closed sherlock-admin closed 11 months ago

sherlock-admin commented 1 year ago

tives

medium

Early Vault depositor can manipulate exchange rates to steal funds from later depositors

Summary

An early user can manipulate the price per share and profit from late users' deposits because of the precision loss caused by the large value of price per share.****

Vulnerability Detail

Part 1: deposit

A malicious early user can deposit 1 wei of token as the first depositor to the Vault, and get 1 wei of shares.

User shares are set in processLocal/processGlobal. These are called in _settle, which is also called in Vault.update(... depositAssets ...) 

function update(
        address account,
        UFixed6 depositAssets,
        UFixed6 redeemShares,
        UFixed6 claimAssets
    ) external whenNotPaused {
        _settleUnderlying();
        Context memory context = _loadContext(account);

        _settle(context);

function _settle()
...
     context.local.processLocal(

function processLocal(
    Account memory self,
    uint256 latestId,
    Checkpoint memory checkpoint,
    UFixed6 deposit,
    UFixed6 redemption
) internal pure {
    self.latest = latestId;
    (self.assets, self.shares) = (
        self.assets.add(checkpoint.toAssetsLocal(redemption)),
        self.shares.add(checkpoint.toSharesLocal(deposit))
    );

_toSharesLocal > _toShares > _withSpread

function _withSpread(Checkpoint memory self, UFixed6 amount) private pure returns (UFixed6) {
    UFixed6 selfAssets = UFixed6Lib.from(self.assets.max(Fixed6Lib.ZERO));
    UFixed6 totalAmount = self.deposit.add(self.redemption.muldiv(selfAssets, self.shares));

    return totalAmount.isZero() ?
        amount :
        amount.muldiv(totalAmount.sub(self.fee.min(totalAmount)), totalAmount);
}

_withSpread sets the shares amount to 1 wei (return totalAmount.isZero() ? amount : …)

Now, the attacker has 1 share for 1 wei of tokens.

Part 2: share value inflation

Then the attacker can send 10000e18 - 1 of asset token to the Vault and inflate the price per share from 1.0000 to an extreme value of 1.0000e22 ( from (1 + 10000e18 - 1) / 1) .

Part 3: redeeming for inflated share price

The _socialize function returns the claim amount. _socialize in turn calls _collateral, which uses asset.balanceOf() to return the collateral amount

_socialize() returns (UFixed6 claimAmount) >  _collateral

function _collateral(Context memory context) public view returns (Fixed6 value) {
    value = Fixed6Lib.from(UFixed6Lib.from(asset.balanceOf()));
    for (uint256 marketId; marketId < context.markets.length; marketId++)
        value = value.add(context.markets[marketId].collateral);
}

This means that the claimAmount is calculated according to asset.balanceOf(). This means that adversary has retained her initial shares but inflated the share price via sending in tokens manually.

She can now receive more tokens than her shares are worth. These tokens come from the later depositors.

Impact

Initial depositor steals from late depositors due to inflated share price.

Code Snippet

/// @notice Returns the real amount of collateral in the vault
/// @return value The real amount of collateral in the vault
function _collateral(Context memory context) public view returns (Fixed6 value) {
    value = Fixed6Lib.from(UFixed6Lib.from(asset.balanceOf()));
    for (uint256 marketId; marketId < context.markets.length; marketId++)
        value = value.add(context.markets[marketId].collateral);
}

https://github.com/sherlock-audit/2023-07-perennial/blob/main/perennial-v2/packages/perennial-vault/contracts/Vault.sol/#L486

Tool used

Manual Review

Recommendation

Require a bigger initial deposit or premint some shares and burn them before the first deposit. This is a well known attack vector for ERC4626 vaults. You can read more about it here

sherlock-admin commented 1 year ago

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

141345 commented:

m

nevillehuang commented 12 months ago

Escalate

This issue is marked as won't fixed in the v1 version of the audit here: https://github.com/sherlock-audit/2023-05-perennial-judging/issues/46

According to sherlocks rules:

In an update contest, issues from the previous contest with wont fix labels are not considered valid.

According to sponsor comment:

We won't be adding a solidity level fix at this time, but we will update our deploy scripts to create an initial deposit

Can you confirm @arjun-io ?

sherlock-admin2 commented 12 months ago

Escalate

This issue is marked as won't fixed in the v1 version of the audit here: https://github.com/sherlock-audit/2023-05-perennial-judging/issues/46

According to sherlocks rules:

In an update contest, issues from the previous contest with wont fix labels are not considered valid.

According to sponsor comment:

We won't be adding a solidity level fix at this time, but we will update our deploy scripts to create an initial deposit

Can you confirm @arjun-io ?

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.

Emedudu commented 12 months ago

Escalate

Inflation of shares is not possible.

During settlement, pending deposits will be converted to shares in the following way: Vault#_settle->Account#processLocal->Checkpoint#toSharesLocal->Checkpoint#_toShares

As we can see, shares is calculated as depositedAssets*checkpoint.shares/checkpoint.assets.

checkpoint.assets is updated here, which calls this.

First depositor bug would have been possible if shares was calculated as: depositedAssets*checkpoint.shares/assets.balanceOf(address(this)).

The _socialize function basically does this: If the amount that user wants to claim is more than total assets available in vault, the user should receive a pro-rata share of the total assets available, so that other users will be able to claim assets as well. Similarly, if the total assets available is greater than the totalAssets legally deposited, _socialize will allow each user to be able to claim more assets because the unaccounted assets will be distributed to legal depositors proportionately.

So if an "attacker" transfers 10000e18 - 1 of asset token as described in the report, and another user legally deposits 1 wei, 1 wei of shares will be minted to user, and _socialize will allow attacker to claim (1/2)10000e18 asset tokens, and the other user will also claim (1/2)10000e18 asset tokens, which means loss for the attacker.

sherlock-admin2 commented 12 months ago

Escalate

Inflation of shares is not possible.

During settlement, pending deposits will be converted to shares in the following way: Vault#_settle->Account#processLocal->Checkpoint#toSharesLocal->Checkpoint#_toShares

As we can see, shares is calculated as depositedAssets*checkpoint.shares/checkpoint.assets.

checkpoint.assets is updated here, which calls this.

First depositor bug would have been possible if shares was calculated as: depositedAssets*checkpoint.shares/assets.balanceOf(address(this)).

The _socialize function basically does this: If the amount that user wants to claim is more than total assets available in vault, the user should receive a pro-rata share of the total assets available, so that other users will be able to claim assets as well. Similarly, if the total assets available is greater than the totalAssets legally deposited, _socialize will allow each user to be able to claim more assets because the unaccounted assets will be distributed to legal depositors proportionately.

So if an "attacker" transfers 10000e18 - 1 of asset token as described in the report, and another user legally deposits 1 wei, 1 wei of shares will be minted to user, and _socialize will allow attacker to claim (1/2)10000e18 asset tokens, and the other user will also claim (1/2)10000e18 asset tokens, which means loss for the attacker.

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 commented 11 months ago

@141345

arjun-io commented 11 months ago

Fixed: https://github.com/equilibria-xyz/perennial-v2/pull/87

hrishibhat commented 11 months ago

Result: Low Unique Considering this generic issue related to vaults was already rewarded in the previous contest and has a Wont fix label, this is a low issue.

sherlock-admin2 commented 11 months ago

Escalations have been resolved successfully!

Escalation status:

arjun-io commented 11 months ago

Note for the fix review: the above fix had a slight bug which is fixed here: https://github.com/equilibria-xyz/perennial-v2/pull/101#pullrequestreview-1630111017