sherlock-audit / 2023-03-notional-judging

12 stars 6 forks source link

xiaoming90 - Liquidators who have cash balance cannot perform any liquidation action #187

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

xiaoming90

medium

Liquidators who have cash balance cannot perform any liquidation action

Summary

Certain liquidators would not be able to perform liquidation against any account, which will slightly affect the market efficiency.

Vulnerability Detail

Liquidators may now liquidate any one of the currencies a vault account is borrowing (primary or secondary). They will deposit underlying tokens into the vault account (minting account cash held) and receive vault shares.

Assume an edge case where the VaultAccountStorage.secondaryCashOne or VaultAccountStorage.secondaryCashTwo of the liquidator's vault account holds some cash. This might happen if someone has liquidated the liquidator's vault account earlier.

When the liquidator receives vault shares, the _transferVaultSharesToLiquidator function will be triggered, which in turn calls the setVaultAccount function against the liquidator's vault account. In this case, the liquidator can no longer perform any liquidation action because Line 118 will always revert because his/her secondary cash is not zero.

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultAccount.sol#L103

File: VaultAccount.sol
103:     function setVaultAccount(
104:         VaultAccount memory vaultAccount,
105:         VaultConfig memory vaultConfig,
106:         bool checkMinBorrow 
107:     ) internal {
108:         mapping(address => mapping(address => VaultAccountStorage)) storage store = LibStorage
109:             .getVaultAccount();
110:         VaultAccountStorage storage s = store[vaultAccount.account][vaultConfig.vault];
111: 
112:         _setVaultAccount(vaultAccount, vaultConfig, s, checkMinBorrow, false);
113: 
114:         // Cash balances should never be preserved after a non-liquidation transaction,
115:         // during enter, exit, roll and settle any cash balances should be applied to
116:         // the transaction. These cash balances are only set after liquidation.
117:         s.primaryCash = 0;
118:         require(s.secondaryCashOne == 0 && s.secondaryCashTwo == 0);
119:     }

This also affects the primaryCash within the vault account. If there is some cash in the primaryCash, then _setVaultAccount() function will revert because vaultAccount.tempCashBalance != 0.

Impact

This is an edge case where certain liquidators would not be able to perform liquidation against any account, which will slightly affect the market efficiency.

Code Snippet

https://github.com/sherlock-audit/2023-03-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultAccount.sol#L103

Tool used

Manual Review

Recommendation

Estimate the number of liquidators that could potentially be affected by this issue. If the number of liquidators affected by this issue is significant, the liquidation capacity of the liquidator community will be reduced, and this issue should be mitigated by updating the implementation to allow liquidators with cash balances to perform liquidation.

On the other hand, if the number of liquidators affected by this issue is insignificant, evaluate internally to determine whether it is worth fixing this issue.

T-Woodward commented 1 year ago

I don't think this is really a vulnerability. Liquidators can just use a clean address.