search

sherlock-audit / 2022-09-notional-judging

4 stars 2 forks source link

xiaoming90 - Deposit Cannot Be Used For Repayment When Rolling Position #80

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

xiaoming90

high

Deposit Cannot Be Used For Repayment When Rolling Position

Summary

The user's deposit cannot be used for repayment when rolling a position.

Vulnerability Detail

During rolling over a position, based on the comments below, it was understood that the vault allows a deposit from the user to be used as repayment for the lending. This is to allow an account to roll its position even if they are close to the max borrow capacity. However, it was observed that it is not possible for the users to do so.

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

File: VaultAccountAction.sol
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:         }

Per the source code below, the deposit is credited into the user's vault account after the repayment. The repayment is executed at Line 122 via the vaultAccount.lendToExitVault function first, and then the user's deposit is credited into their account at Line 138 via the depositForRollPosition function.

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

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:     }

Impact

Users will not be able to roll their position if there are close to the max borrow capacity. Thus, their options are only limited to exiting the existing vault OR waiting until the existing vault has matured/settled before exiting. If these options are suboptimal, this will result in a loss of gain/assets for the users because they would have been able to gain more assets if their positions were rolled over.

Code Snippet

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

Tool used

Manual Review

Recommendation

It is recommended to update the rollVaultPosition implementation to allow the user's deposit to be used for repayment.

function rollVaultPosition(
    address account,
    address vault,
    uint256 fCashToBorrow,
    uint256 maturity,
    uint256 depositAmountExternal,
    uint32 minLendRate,
    uint32 maxBorrowRate,
    bytes calldata enterVaultData
) external payable override nonReentrant returns (uint256 strategyTokensAdded) {
    VaultConfig memory vaultConfig = VaultConfiguration.getVaultConfigStateful(vault);
    vaultConfig.authorizeCaller(account, VaultConfiguration.ONLY_VAULT_ROLL);

    // If the vault allows further re-entrancy then set the status back to the default
    if (vaultConfig.getFlag(VaultConfiguration.ALLOW_REENTRANCY)) {
        reentrancyStatus = _NOT_ENTERED;
    }

    VaultAccount memory vaultAccount = VaultAccountLib.getVaultAccount(account, vault);
    // Cannot roll unless all of these requirements are met
    require(
        vaultConfig.getFlag(VaultConfiguration.ENABLED) &&
        vaultConfig.getFlag(VaultConfiguration.ALLOW_ROLL_POSITION) &&
        block.timestamp < vaultAccount.maturity && // cannot have matured yet
        vaultAccount.maturity < maturity && // new maturity must be forward in time
        fCashToBorrow > 0, // must borrow into the next maturity, if not, then they should just exit
        "No Roll Allowed"
    );

    VaultState memory vaultState = VaultStateLib.getVaultState(vaultConfig.vault, vaultAccount.maturity);
    // Exit the maturity pool by removing all the vault shares. All of the strategy tokens will be
    // re-deposited into the new maturity
    uint256 strategyTokens = vaultState.exitMaturity(vaultAccount, vaultAccount.vaultShares);

+    // Takes a deposit from the user as repayment for the lending, allows an account to roll their position
+    // even if they are close to the max borrow capacity.
+    if (depositAmountExternal > 0) {
+        vaultAccount.depositForRollPosition(vaultConfig, depositAmountExternal);
+    }

    // Exit the vault first and debit the temporary cash balance with the cost to exit
    vaultAccount.lendToExitVault(
        vaultConfig,
        vaultState,
        vaultAccount.fCash.neg(), // must fully exit the fCash position
        minLendRate,
        block.timestamp
    );

    // This should never be the case for a healthy vault account due to the mechanics of exiting the vault
    // above but we check it for safety here.
    require(vaultAccount.fCash == 0, "Failed Lend");
    vaultState.setVaultState(vaultConfig.vault);

-    // Takes a deposit from the user as repayment for the lending, allows an account to roll their position
-    // even if they are close to the max borrow capacity.
-    if (depositAmountExternal > 0) {
-        vaultAccount.depositForRollPosition(vaultConfig, depositAmountExternal);
-    }

    // Enters the vault at the longer dated maturity
    strategyTokensAdded = vaultAccount.borrowAndEnterVault(
        vaultConfig,
        maturity, // This is the new maturity to enter
        fCashToBorrow,
        maxBorrowRate,
        enterVaultData,
        strategyTokens,
        0 // No additional tokens deposited in this method
    );

    emit VaultRollPosition(vault, account, maturity, fCashToBorrow);
}
jeffywu commented 1 year ago

Can the auditor provide a POC that the deposit is not applied to repayment? There is an existing unit tests that validates that this does happen: https://github.com/notional-finance/contracts-v2/blob/bcb145c725d90e65f99505d44275e62f37398264/tests/stateful/vaults/test_vault_roll.py#L240-L241

The reason is that the account accrues a positive or negative net cash balance throughout the transaction so the placement of the deposit within the parent function does not have an effect. I believe this report is invalid unless the audtor can provide a POC.