search

sherlock-audit / 2022-09-notional-judging

4 stars 2 forks source link

csanuragjain - One vault can enter another vault #113

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

csanuragjain

medium

One vault can enter another vault

Summary

The ALLOW_REENTRANCY flag, if turned on, can disable reentrancy protection of selected vaults. However there is no check to see that these selected vaults are not reentering the reentrancy protected vaults

Vulnerability Detail

  1. Assume there are 2 vaults A,B where vault A has ALLOW_REENTRANCY as true
  2. User calls enterVault with Vault A
  3. Since vaultConfig.getFlag(VaultConfiguration.ALLOW_REENTRANCY) is true for Vault A so Reentrancy is allowed
  4. Since reentrancy is allowed for Vault A, reentrancyStatus is reset to _NOT_ENTERED. This means if this contract interact with any malicious contract then that contract can again call enterVault with Vault B address in mid way of execution even though this transaction should only allow Vault A

Impact

You can reenter a protected vault from a non protected vault

Code Snippet

https://github.com/notional-finance/contracts-v2/blob/cf05d8e3e4e4feb0b0cef2c3f188c91cdaac38e0/contracts/external/actions/VaultAccountAction.sol#L49

Tool used

Manual Review

Recommendation

One more flag should be added mentioning the Vault for which reentrancy was allowed, say vaultAllowed=A. On enterVault execution, if reentrancyStatus is _NOT_ENTERED then revert if vault!=vaultAllowed

Duplicate of #12

jeffywu commented 1 year ago

This attack assumes that the vault somehow passes control of the execution back to the caller, not clear how that would happen except for a malicious vault or some transfer of control on an ERC20.