Malicious User Can Steal All Assets From A Vault When Exiting The Vault By Performing A Re-Entrancy Attack

Summary

An attacker can perform a re-entrancy attack against the vault to drain all its assets by exploiting the vulnerable exit vault 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.exitVault function allows re-entrancy. Line 183 within the VaultAccountAction.exitVault function will set the reentrancyStatus back to _NOT_ENTERED to allow re-entrancy.

Assume that a vault allows re-entrancy and Bob (attacker) is trying to exit the vault post-maturity after the vault has settled by calling the VaultAccountAction.exitVault function. Bob has 100 vault shares, so in the storage, Bob's vaultAccount.vaultShares = 100 at this point.

At Line 186, the VaultAccountLib.getVaultAccount function is called to load the user'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.

At Line 192, the vaultAccount.settleVaultAccount function will settle Bob's vault account and set the vaultAccount.vaultShares to 0 in the memory.

At Line 204, the vaultConfig.redeemWithDebtRepayment function will be called to redeem all strategy tokens, and any profits will be sent back to the user. Within the vaultConfig.redeemWithDebtRepayment function, Ether or ERC20 tokens will be transferred to the user depending on the vault's underlying assets. The transfer will effectively pass the control back to the user. At this point, the vaultAccount.vaultShares is 0 in the memory, but the vaultAccount.vaultShares is still 100 in the storage because the vaultAccount.setVaultAccount has not been triggered yet to write the information stored in the memory back to the storage.

Bob re-enters the VaultAccountAction.exitVault function again, and at Line 186, the VaultAccountLib.getVaultAccount function is called again to load the user's vault account data from the storage and load them onto the vaultAccount variable on memory. Note that the vaultAccount.vaultShares is still 100 in storage at this point. Therefore, in this context, the vaultAccount.vaultShares in the memory will be 100, and the vault will redeem the 100 vault shares again and transfer the profits to Bob again.

Bob repeats the above steps multiple times until all the assets in the vault are drained.

At the end of the VaultAccountAction.exitVault function at Line 247, the vaultAccount in memory is finally written back to the storage via the vaultAccount.setVaultAccount function.

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 the profit back to users 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.

https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/external/actions/VaultAccountAction.sol#L169

File: VaultAccountAction.sol
169:     function exitVault(
170:         address account,
171:         address vault,
172:         address receiver,
173:         uint256 vaultSharesToRedeem,
174:         uint256 fCashToLend,
175:         uint32 minLendRate,
176:         bytes calldata exitVaultData
177:     ) external payable override nonReentrant returns (uint256 underlyingToReceiver) {
178:         VaultConfig memory vaultConfig = VaultConfiguration.getVaultConfigStateful(vault);
179:         vaultConfig.authorizeCaller(account, VaultConfiguration.ONLY_VAULT_EXIT);
180: 
181:         // If the vault allows further re-entrancy then set the status back to the default
182:         if (vaultConfig.getFlag(VaultConfiguration.ALLOW_REENTRANCY)) {
183:             reentrancyStatus = _NOT_ENTERED;
184:         }
185: 
186:         VaultAccount memory vaultAccount = VaultAccountLib.getVaultAccount(account, vault);
187: 
188:         if (vaultAccount.maturity <= block.timestamp) {
189:             // Save this off because settleVaultAccount will clear the maturity
190:             uint256 maturity = vaultAccount.maturity;
191:             // Past maturity, an account will be settled instead
192:             uint256 strategyTokens = vaultAccount.settleVaultAccount(vaultConfig, block.timestamp);
193: 
194:             if (vaultAccount.tempCashBalance > 0) {
195:                 // Transfer asset cash back to the account
196:                 VaultConfiguration.transferFromNotional(
197:                     receiver, vaultConfig.borrowCurrencyId, vaultAccount.tempCashBalance
198:                 );
199:                 vaultAccount.tempCashBalance = 0;
200:             }
201: 
202:             // Redeems all strategy tokens and any profits are sent back to the account, it is possible for temp
203:             // cash balance to be negative here if the account is insolvent
204:             underlyingToReceiver = vaultConfig.redeemWithDebtRepayment(
205:                 vaultAccount, receiver, strategyTokens, maturity, exitVaultData
206:             );
207:             emit VaultExitPostMaturity(vault, account, maturity, underlyingToReceiver);
208:         } else {
..SNIP..
241:         }
242: 
243:         if (vaultAccount.fCash == 0 && vaultAccount.vaultShares == 0) {
244:             // If the account has no position in the vault at this point, set the maturity to zero as well
245:             vaultAccount.maturity = 0;
246:         }
247:         vaultAccount.setVaultAccount(vaultConfig);
248:     }

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.

https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultAccount.sol#L39

File: VaultAccount.sol
38:     /// @notice Returns a single account's vault position
39:     function getVaultAccount(
40:         address account, address vault
41:     ) internal view returns (VaultAccount memory vaultAccount) {
42:         mapping(address => mapping(address => VaultAccountStorage)) storage store = LibStorage.getVaultAccount();
43:         VaultAccountStorage storage s = store[account][vault];
44: 
45:         // fCash is negative on the stack
46:         vaultAccount.fCash = -int256(uint256(s.fCash));
47:         vaultAccount.maturity = s.maturity;
48:         vaultAccount.vaultShares = s.vaultShares;
49:         vaultAccount.account = account;
50:         vaultAccount.tempCashBalance = 0;
51:     }

https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/internal/vaults/VaultAccount.sol#L54

File: VaultAccount.sol
53:     /// @notice Sets a single account's vault position in storage
54:     function setVaultAccount(VaultAccount memory vaultAccount, VaultConfig memory vaultConfig) internal {
55:         mapping(address => mapping(address => VaultAccountStorage)) storage store = LibStorage
56:             .getVaultAccount();
57:         VaultAccountStorage storage s = store[vaultAccount.account][vaultConfig.vault];
..SNIP..
77:         s.fCash = vaultAccount.fCash.neg().toUint().toUint80();
78:         s.vaultShares = vaultAccount.vaultShares.toUint80();
79:         s.maturity = vaultAccount.maturity.toUint32();
80:     }

Impact

All assets within the vault can be drained by the attacker, thus leaving other vault shareholders with nothing.

Code Snippet

https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/external/actions/VaultAccountAction.sol#L169

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 performing a transfer (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

sherlock-audit / 2022-09-notional-judging

xiaoming90 - Malicious User Can Steal All Assets From A Vault When Exiting The Vault By Performing A Re-Entrancy Attack #85