search

sherlock-audit / 2023-05-perennial-judging

12 stars 9 forks source link

josephdara - user deposits are overwritten #234

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

josephdara

high

user deposits are overwritten

Summary

In BalancedVault.sol user deposits mapping is overwritten anytime function _settle(address account) is called.

Vulnerability Detail

function _settle(address account) is a Hook that is called before every stateful operation and it Settles the vault's account on both the long and short product, along with any global or user-specific deposits/redemptions. But it directly overwrites user deposits with what ever pending deposits are there without consideration for current user deposits

        if (account != address(0)) {
            if (accountContext.epoch > _latestEpochs[account]) {
                _delayedMintAccount(account, _balanceOfAtEpoch(accountContext, account).sub(_balanceOf[account].add(_pendingRedemptions[account])));
                _unclaimed[account] = _unclaimedAtEpoch(accountContext, account);
                _deposits[account] = UFixed18Lib.ZERO;
                _redemptions[account] = UFixed18Lib.ZERO;
                _latestEpochs[account] = accountContext.epoch;
            }
             if (accountContext.epoch > _pendingEpochs[account]) {
             //@audit-issue _deposits mapping overwritten
                _deposits[account] = _pendingDeposits[account];
                _redemptions[account] = _pendingRedemptions[account];
                _latestEpochs[account] = _pendingEpochs[account];
                _pendingDeposits[account] = UFixed18Lib.ZERO;
                _pendingRedemptions[account] = UFixed18Lib.ZERO;
                _pendingEpochs[account] = accountContext.epoch;

Impact

Users making multiple deposits will loose there previous deposits if currentEpochStale() is true. A malicious attacker can also call depositFor to overwrite an accounts balance if currentEpochStale() is true

Code Snippet

https://github.com/sherlock-audit/2023-05-perennial/blob/main/perennial-mono/packages/perennial-vaults/contracts/balanced/BalancedVault.sol#L375-L422

Tool used

Manual Review

Recommendation

change the function to be 

   if (accountContext.epoch > _pendingEpochs[account]) {
             //@audit-issue _deposits mapping overwritten
++                _deposits[account].add( _pendingDeposits[account]);
                _redemptions[account] = _pendingRedemptions[account];
                _latestEpochs[account] = _pendingEpochs[account];
                _pendingDeposits[account] = UFixed18Lib.ZERO;
                _pendingRedemptions[account] = UFixed18Lib.ZERO;
                _pendingEpochs[account] = accountContext.epoch;