Assets In A Vault Account That Needs To Be Deleveraged Can Be Stolen Via Re-Entrancy Attack
Summary
An attacker can perform a re-entrancy attack against a vault account that needs to be deleveraged to steal all the assets within it by exploiting the vulnerable deleverage account function as the checks-effects-interactions pattern is not adhered to.
Vulnerability Detail
If theVaultConfiguration.ALLOW_REENTRANCY setting of a vault is set to True, the VaultAccountAction.deleverageAccount function allows re-entrancy. The VaultAccountAction._authenticateDeleverage function called at Line 269 of the VaultAccountAction.deleverageAccount function will set the reentrancyStatus back to _NOT_ENTERED to allow re-entrancy.
Assume the following:
A vault allows re-entrancy and Bob (attacker) is trying to deleverage Alice's account
Alice has 100 vault shares, so in the storage, Bob's vaultAccount.vaultShares = 100 at this point
30 of Alice's vault shares need to be deleveraged/liquidated to bring her account back to healthy collateral ratio
Bob calls the VaultAccountAction.deleverageAccount function in an attempt to deleverage 30 of Alice's vault shares with the appropriate amount of cash/deposit.
At Line 270, the VaultAccountLib.getVaultAccount function is called to load Alice's vault account data from the storage and load them onto the vaultAccount variable on memory. The key point to note is that vaultAccount variable is stored in memory. Alice's vaultAccount.vaultShares = 100 at this point.
At Line 284, the VaultAccountAction._depositLiquidatorAmount function will be triggered to pull the deposit from Bob's address. An important point here is that within the VaultAccountAction._depositLiquidatorAmount, it will perform a ERC20.transferFrom call with Bob's address in the from parameter. Depending on the tokens to be transferred as a deposit, some tokens will pass the control to the sender (Bob). For instance, ERC777 contains the tokensToSend hook that will call the sender when tokens are about to be moved.
Assume that the control is passed to Bob. Bob re-enters the VaultAccountAction.deleverageAccount function again, and at Line 270, the VaultAccountLib.getVaultAccount function is called again to load Alice's vault account data from the storage and load them onto the vaultAccount variable on memory. Note that Alice's vaultAccount.vaultShares is still 100 in storage at this point. Therefore, in this context, the vaultAccount.vaultShares in the memory will be 100. The calculateCollateralRatio function at Line 277 will perform is calculated based on the fact that Alice still holds 100 vault shares and determine that 30 of Alice's vault shares need to be deleveraged to bring her account back to healthy collateral ratio. Therefore, Bob can deleverage 30 of Alice's vault shares again for a second time.
Bob repeats the above steps multiple times until all of Alice's assets are drained.
At the end of the VaultAccountAction.deleverageAccount function at Line 317, the vaultAccount in memory is finally written back to the storage via the vaultAccount.setVaultAccount function.
Finally, Alice's vault shares or assets will be transferred to Bob in lines 321-328.
A checks-effects-interactions pattern is a pattern to avoid a re-entrancy attack. Note that in terms of the checks-effects-interactions pattern:
The process of writing the data from memory back to storage will be classified as the "effects" step.
The process of transferring/pulling the deposit from the caller's address will be classified as the "interactions" step
Notice that over here the classic checks-effects-interactions pattern is not followed because the "effects" step occurred at the end, after the "interactions" step. Thus, a re-entrancy attack is possible over here.
File: VaultAccountAction.sol
261: function deleverageAccount(
262: address account,
263: address vault,
264: address liquidator,
265: uint256 depositAmountExternal,
266: bool transferSharesToLiquidator,
267: bytes calldata redeemData
268: ) external nonReentrant override returns (uint256 profitFromLiquidation) {
269: VaultConfig memory vaultConfig = _authenticateDeleverage(account, vault, liquidator);
270: VaultAccount memory vaultAccount = VaultAccountLib.getVaultAccount(account, vault);
271: VaultState memory vaultState = VaultStateLib.getVaultState(vault, vaultAccount.maturity);
272:
273: // Check that the account has an active position. After maturity, accounts will be settled instead.
274: require(block.timestamp < vaultAccount.maturity);
275:
276: // Check that the collateral ratio is below the minimum allowed
277: (int256 collateralRatio, int256 vaultShareValue) = vaultConfig.calculateCollateralRatio(
278: vaultState, account, vaultAccount.vaultShares, vaultAccount.fCash
279: );
280: require(collateralRatio < vaultConfig.minCollateralRatio , "Sufficient Collateral");
281:
282: Token memory assetToken = TokenHandler.getAssetToken(vaultConfig.borrowCurrencyId);
283: // This will deposit some amount of cash into vaultAccount.tempCashBalance
284: _depositLiquidatorAmount(
285: liquidator, assetToken, vaultAccount, vaultConfig, depositAmountExternal, vaultShareValue
286: );
287:
288: // The liquidator will purchase vault shares from the vault account at discount. The calculation is:
289: // (cashDeposited / assetCashValueOfShares) * liquidationRate * vaultShares
290: // where cashDeposited / assetCashValueOfShares represents the share of the total vault share
291: // value the liquidator has deposited
292: // and liquidationRate is a percentage greater than 100% that represents their bonus
293: uint256 vaultSharesToLiquidator;
294: {
295: vaultSharesToLiquidator = vaultAccount.tempCashBalance.toUint()
296: .mul(vaultConfig.liquidationRate.toUint())
297: .mul(vaultAccount.vaultShares)
298: .div(vaultShareValue.toUint())
299: .div(uint256(Constants.RATE_PRECISION));
300: }
301:
302: vaultAccount.vaultShares = vaultAccount.vaultShares.sub(vaultSharesToLiquidator);
303: // The liquidated account will lend to exit their position at a zero interest rate and forgo any future interest
304: // from asset tokens. Trading on the AMM during liquidation is risky and lending at a zero interest rate is more
305: // costly to the the liquidated account but is safer from a protocol perspective. This can be seen as a protocol
306: // level liquidation fee.
307: {
308: int256 fCashToReduce = vaultConfig.assetRate.convertToUnderlying(vaultAccount.tempCashBalance);
309: vaultAccount.updateAccountfCash(vaultConfig, vaultState, fCashToReduce, vaultAccount.tempCashBalance.neg());
310: // _calculateLiquidatorDeposit should ensure that we only ever lend up to a zero balance, but in the
311: // case of any off by one issues we clear the fCash balance by down to zero.
312: if (vaultAccount.fCash > 0) vaultAccount.fCash = 0;
313: emit VaultDeleverageAccount(vault, account, vaultSharesToLiquidator, fCashToReduce);
314: }
315:
316: // Sets the liquidated account account
317: vaultAccount.setVaultAccount(vaultConfig);
318:
319: // Redeems the vault shares for asset cash and transfers it to the designated address
320: emit VaultLiquidatorProfit(vault, account, liquidator, vaultSharesToLiquidator, transferSharesToLiquidator);
321: if (transferSharesToLiquidator) {
322: vaultState.setVaultState(vaultConfig.vault);
323: profitFromLiquidation = _transferLiquidatorProfits(liquidator, vaultConfig, vaultSharesToLiquidator, vaultAccount.maturity);
324: } else {
325: profitFromLiquidation = _redeemLiquidatorProfits(
326: liquidator, vaultConfig, vaultState, vaultSharesToLiquidator, redeemData, assetToken
327: );
328: }
329: }
If theVaultConfiguration.ALLOW_REENTRANCY setting of a vault is set to True, this function will set the reentrancyStatus back to _NOT_ENTERED to allow re-entrancy.
File: VaultAccountAction.sol
331: /// @notice Authenticates a call to the deleverage method
332: function _authenticateDeleverage(
333: address account,
334: address vault,
335: address liquidator
336: ) private returns (VaultConfig memory vaultConfig) {
337: vaultConfig = VaultConfiguration.getVaultConfigStateful(vault);
338: // If the vault allows further re-entrancy then set the status back to the default
339: if (vaultConfig.getFlag(VaultConfiguration.ALLOW_REENTRANCY)) {
340: reentrancyStatus = _NOT_ENTERED;
341: }
342:
343: // Authorization rules for deleveraging
344: if (vaultConfig.getFlag(VaultConfiguration.ONLY_VAULT_DELEVERAGE)) {
345: require(msg.sender == vault, "Unauthorized");
346: } else {
347: require(msg.sender == liquidator, "Unauthorized");
348: }
349:
350: // Cannot liquidate self, if a vault needs to deleverage itself as a whole it has other methods
351: // in VaultAction to do so.
352: require(account != msg.sender && account != liquidator, "Unauthorized");
353: }
Additional Note About Vault Account Data
All vault account data are stored inside a storage slot within the LibStorage. At the start of the function, the function will attempt to load the vault account data from the storage slot within the LibStorage via the VaultAccount.getVaultAccount function , and load them onto a state variable in memory (usually called vaultAccount). When logic within a function is executed, all the changes are made against the vaultAccount state variable in the memory. At the end of the function, the function will write the data from vaultAccount state variable in the memory back onto the storage slot in the LibStorage via the VaultAccount.setVaultAccount function.
It is recommended to adhere to the checks-effects-interactions pattern to prevent re-entrancy attacks. Write the vault account data in the memory back to the storage (effects) before transferring/pulling the deposit from the callers (interactions).
If possible, remove the option of allowing re-entrancy as this is a significant attack vector that an attacker always exploits. Redesign the system so that re-entrancy is not needed.
xiaoming90
high
Assets In A Vault Account That Needs To Be Deleveraged Can Be Stolen Via Re-Entrancy Attack
Summary
An attacker can perform a re-entrancy attack against a vault account that needs to be deleveraged to steal all the assets within it by exploiting the vulnerable deleverage account function as the checks-effects-interactions pattern is not adhered to.
Vulnerability Detail
If the
VaultConfiguration.ALLOW_REENTRANCYsetting of a vault is set toTrue, theVaultAccountAction.deleverageAccountfunction allows re-entrancy. TheVaultAccountAction._authenticateDeleveragefunction called at Line 269 of theVaultAccountAction.deleverageAccountfunction will set thereentrancyStatusback to_NOT_ENTEREDto allow re-entrancy.Assume the following:
vaultAccount.vaultShares = 100at this pointBob calls the
VaultAccountAction.deleverageAccountfunction in an attempt to deleverage 30 of Alice's vault shares with the appropriate amount of cash/deposit.At Line 270, the
VaultAccountLib.getVaultAccountfunction is called to load Alice's vault account data from the storage and load them onto thevaultAccountvariable on memory. The key point to note is thatvaultAccountvariable is stored in memory. Alice'svaultAccount.vaultShares = 100at this point.At Line 284, the
VaultAccountAction._depositLiquidatorAmountfunction will be triggered to pull the deposit from Bob's address. An important point here is that within theVaultAccountAction._depositLiquidatorAmount, it will perform aERC20.transferFromcall with Bob's address in thefromparameter. Depending on the tokens to be transferred as a deposit, some tokens will pass the control to the sender (Bob). For instance, ERC777 contains thetokensToSendhook that will call the sender when tokens are about to be moved.Assume that the control is passed to Bob. Bob re-enters the
VaultAccountAction.deleverageAccountfunction again, and at Line 270, theVaultAccountLib.getVaultAccountfunction is called again to load Alice's vault account data from the storage and load them onto thevaultAccountvariable on memory. Note that Alice'svaultAccount.vaultSharesis still100in storage at this point. Therefore, in this context, thevaultAccount.vaultSharesin the memory will be100. ThecalculateCollateralRatiofunction at Line 277 will perform is calculated based on the fact that Alice still holds 100 vault shares and determine that 30 of Alice's vault shares need to be deleveraged to bring her account back to healthy collateral ratio. Therefore, Bob can deleverage 30 of Alice's vault shares again for a second time.Bob repeats the above steps multiple times until all of Alice's assets are drained.
At the end of the
VaultAccountAction.deleverageAccountfunction at Line 317, thevaultAccountin memory is finally written back to the storage via thevaultAccount.setVaultAccountfunction.Finally, Alice's vault shares or assets will be transferred to Bob in lines 321-328.
A checks-effects-interactions pattern is a pattern to avoid a re-entrancy attack. Note that in terms of the checks-effects-interactions pattern:
Notice that over here the classic checks-effects-interactions pattern is not followed because the "effects" step occurred at the end, after the "interactions" step. Thus, a re-entrancy attack is possible over here.
https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/external/actions/VaultAccountAction.sol#L261
If the
VaultConfiguration.ALLOW_REENTRANCYsetting of a vault is set toTrue, this function will set thereentrancyStatusback to_NOT_ENTEREDto allow re-entrancy.https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/external/actions/VaultAccountAction.sol#L332
Additional Note About Vault Account Data
All vault account data are stored inside a storage slot within the LibStorage. At the start of the function, the function will attempt to load the vault account data from the storage slot within the LibStorage via the
VaultAccount.getVaultAccountfunction , and load them onto a state variable in memory (usually calledvaultAccount). When logic within a function is executed, all the changes are made against thevaultAccountstate variable in the memory. At the end of the function, the function will write the data fromvaultAccountstate variable in the memory back onto the storage slot in the LibStorage via theVaultAccount.setVaultAccountfunction.https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultAccount.sol#L39
https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultAccount.sol#L54
Impact
All assets within a vault account that needs to be deleveraged can be stolen by the attacker
Code Snippet
https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/external/actions/VaultAccountAction.sol#L261 https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/external/actions/VaultAccountAction.sol#L332 https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultAccount.sol#L39 https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultAccount.sol#L54
Tool used
Manual Review
Recommendation
It is recommended to adhere to the checks-effects-interactions pattern to prevent re-entrancy attacks. Write the vault account data in the memory back to the storage (effects) before transferring/pulling the deposit from the callers (interactions).
If possible, remove the option of allowing re-entrancy as this is a significant attack vector that an attacker always exploits. Redesign the system so that re-entrancy is not needed.
Duplicate of #12