Users Can Gain Additional Vault Shares When Rolling Position Via Re-Entrancy Attack
Summary
An attacker can perform a re-entrancy attack to gain additional vault shares when rolling over their position to a longer dated maturity by exploiting the roll vault position 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.rollVaultPosition function allows re-entrancy. Line 102 within the VaultAccountAction.rollVaultPosition 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 roll over its position to a longer dated maturity by calling the VaultAccountAction.rollVaultPosition function. Bob has 100 vault shares, so in the storage, Bob's vaultAccount.vaultShares = 100 at this point.
At Line 105, 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 119, the vaultState.exitMaturity function will be triggered to exit the current maturity by removing all existing 100 vault shares.
At Line 138, the vaultAccount.depositForRollPosition function will be triggered to pull the necessary deposit from Bob's address as repayment. An important point here is that within the vaultAccount.depositForRollPosition, 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.rollVaultPosition function again, and at Line 105, the VaultAccountLib.getVaultAccount function is called again to load Bob's vault account data from the storage and load them onto the vaultAccount variable on memory. Note that Bob's vaultAccount.vaultShares is still 100 in storage at this point.
Then, the vaultState.exitMaturity function at Line 119 will be triggered again for the second time. Note that within the vaultState.exitMaturity function, Bob's vaultAccount.vaultShares is still 100 because the state changes are not written back to the storage yet, so the math operation within the function will not revert, and it will be able to redeem the 100 vault shares successfully.
At Line 142, the vaultAccount.borrowAndEnterVault function will be triggered, and Bob's position will be moved to a longer dated maturity. When the inner re-entrancy call returns, the code execution flow will resume from Line 138. Subsequently, the vaultAccount.borrowAndEnterVault function at Line 142 will be triggered again for the second time, thus moving Bob's position to a longer dated maturity again.
At the end of the VaultAccountAction.borrowAndEnterVault function, the vaultAccount in memory is finally written back to the storage via the vaultAccount.setVaultAccount function.
As such, Bob effectively gains twice the amount of vault shares by rolling over his position. Bob could gain more by repeating the re-enter multiple times while executing the attack.
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 for repayment 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
087: function rollVaultPosition(
088: address account,
089: address vault,
090: uint256 fCashToBorrow,
091: uint256 maturity,
092: uint256 depositAmountExternal,
093: uint32 minLendRate,
094: uint32 maxBorrowRate,
095: bytes calldata enterVaultData
096: ) external payable override nonReentrant returns (uint256 strategyTokensAdded) {
097: VaultConfig memory vaultConfig = VaultConfiguration.getVaultConfigStateful(vault);
098: vaultConfig.authorizeCaller(account, VaultConfiguration.ONLY_VAULT_ROLL);
099:
100: // If the vault allows further re-entrancy then set the status back to the default
101: if (vaultConfig.getFlag(VaultConfiguration.ALLOW_REENTRANCY)) {
102: reentrancyStatus = _NOT_ENTERED;
103: }
104:
105: VaultAccount memory vaultAccount = VaultAccountLib.getVaultAccount(account, vault);
106: // Cannot roll unless all of these requirements are met
107: require(
108: vaultConfig.getFlag(VaultConfiguration.ENABLED) &&
109: vaultConfig.getFlag(VaultConfiguration.ALLOW_ROLL_POSITION) &&
110: block.timestamp < vaultAccount.maturity && // cannot have matured yet
111: vaultAccount.maturity < maturity && // new maturity must be forward in time
112: fCashToBorrow > 0, // must borrow into the next maturity, if not, then they should just exit
113: "No Roll Allowed"
114: );
115:
116: VaultState memory vaultState = VaultStateLib.getVaultState(vaultConfig.vault, vaultAccount.maturity);
117: // Exit the maturity pool by removing all the vault shares. All of the strategy tokens will be
118: // re-deposited into the new maturity
119: uint256 strategyTokens = vaultState.exitMaturity(vaultAccount, vaultAccount.vaultShares);
120:
121: // Exit the vault first and debit the temporary cash balance with the cost to exit
122: vaultAccount.lendToExitVault(
123: vaultConfig,
124: vaultState,
125: vaultAccount.fCash.neg(), // must fully exit the fCash position
126: minLendRate,
127: block.timestamp
128: );
129:
130: // This should never be the case for a healthy vault account due to the mechanics of exiting the vault
131: // above but we check it for safety here.
132: require(vaultAccount.fCash == 0, "Failed Lend");
133: vaultState.setVaultState(vaultConfig.vault);
134:
135: // Takes a deposit from the user as repayment for the lending, allows an account to roll their position
136: // even if they are close to the max borrow capacity.
137: if (depositAmountExternal > 0) {
138: vaultAccount.depositForRollPosition(vaultConfig, depositAmountExternal);
139: }
140:
141: // Enters the vault at the longer dated maturity
142: strategyTokensAdded = vaultAccount.borrowAndEnterVault(
143: vaultConfig,
144: maturity, // This is the new maturity to enter
145: fCashToBorrow,
146: maxBorrowRate,
147: enterVaultData,
148: strategyTokens,
149: 0 // No additional tokens deposited in this method
150: );
151:
152: emit VaultRollPosition(vault, account, maturity, fCashToBorrow);
153: }
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.
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
Malicious users can gain additional vault shares when rolling over their position to a longer dated maturity. Since the malicious user's number of vault shares is inflated, the value of the vault shares owned by other shareholders will be diluted. Thus, loss of assets for the other vault shareholders.
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
Users Can Gain Additional Vault Shares When Rolling Position Via Re-Entrancy Attack
Summary
An attacker can perform a re-entrancy attack to gain additional vault shares when rolling over their position to a longer dated maturity by exploiting the roll vault position 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.rollVaultPositionfunction allows re-entrancy. Line 102 within theVaultAccountAction.rollVaultPositionfunction will set thereentrancyStatusback to_NOT_ENTEREDto allow re-entrancy.Assume that a vault allows re-entrancy and Bob (attacker) is trying to roll over its position to a longer dated maturity by calling the
VaultAccountAction.rollVaultPositionfunction. Bob has 100 vault shares, so in the storage, Bob'svaultAccount.vaultShares = 100at this point.At Line 105, the
VaultAccountLib.getVaultAccountfunction is called to load the user'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.At Line 119, the
vaultState.exitMaturityfunction will be triggered to exit the current maturity by removing all existing 100 vault shares.At Line 138, the
vaultAccount.depositForRollPositionfunction will be triggered to pull the necessary deposit from Bob's address as repayment. An important point here is that within thevaultAccount.depositForRollPosition, 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.rollVaultPositionfunction again, and at Line 105, theVaultAccountLib.getVaultAccountfunction is called again to load Bob's vault account data from the storage and load them onto thevaultAccountvariable on memory. Note that Bob'svaultAccount.vaultSharesis still100in storage at this point.Then, the
vaultState.exitMaturityfunction at Line 119 will be triggered again for the second time. Note that within thevaultState.exitMaturityfunction, Bob'svaultAccount.vaultSharesis still 100 because the state changes are not written back to the storage yet, so the math operation within the function will not revert, and it will be able to redeem the 100 vault shares successfully.At Line 142, the
vaultAccount.borrowAndEnterVaultfunction will be triggered, and Bob's position will be moved to a longer dated maturity. When the inner re-entrancy call returns, the code execution flow will resume from Line 138. Subsequently, thevaultAccount.borrowAndEnterVaultfunction at Line 142 will be triggered again for the second time, thus moving Bob's position to a longer dated maturity again.At the end of the
VaultAccountAction.borrowAndEnterVaultfunction, thevaultAccountin memory is finally written back to the storage via thevaultAccount.setVaultAccountfunction.As such, Bob effectively gains twice the amount of vault shares by rolling over his position. Bob could gain more by repeating the re-enter multiple times while executing the attack.
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#L87
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
Malicious users can gain additional vault shares when rolling over their position to a longer dated maturity. Since the malicious user's number of vault shares is inflated, the value of the vault shares owned by other shareholders will be diluted. Thus, loss of assets for the other vault shareholders.
Code Snippet
https://github.com/sherlock-audit/2022-09-notional/blob/main/contracts-v2/contracts/external/actions/VaultAccountAction.sol#L87 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