Lack of Slippage Controls in retrieveTokensForWithdraw Function

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-04-noya/blob/main/contracts/accountingManager/AccountingManager.sol#L548

Vulnerability details

Impact

Without slippage controls in the retrieveTokensForWithdraw function, there is a risk of financial loss for users during token retrieval from connectors. This function handles sensitive operations where base tokens are pulled from external liquidity pools or other decentralized financial instruments to fulfill withdrawal requests. In volatile market conditions, the absence of slippage controls can lead to a significant discrepancy between the expected and actual amounts of tokens retrieved, potentially resulting in users receiving less value than their withdrawal entitlements.

Proof of Concept

The function interacts with connectors to retrieve tokens without ensuring the retrieval amounts are within acceptable slippage ranges

https://github.com/code-423n4/2024-04-noya/blob/main/contracts/accountingManager/AccountingManager.sol#L548

   function retrieveTokensForWithdraw(RetrieveData[] calldata retrieveData) public onlyManager nonReentrant {
       uint256 amountAskedForWithdraw_temp = 0;
       uint256 neededAssets = neededAssetsForWithdraw();
       for (uint256 i = 0; i < retrieveData.length; i++) {
           if (!registry.isAnActiveConnector(vaultId, retrieveData[i].connectorAddress)) {
               continue;
           }
           uint256 balanceBefore = baseToken.balanceOf(address(this));
           uint256 amount = IConnector(retrieveData[i].connectorAddress).sendTokensToTrustedAddress(
               address(baseToken), retrieveData[i].withdrawAmount, address(this), retrieveData[i].data
           );
           uint256 balanceAfter = baseToken.balanceOf(address(this));
           if (balanceBefore + amount > balanceAfter) revert NoyaAccounting_banalceAfterIsNotEnough();
           amountAskedForWithdraw_temp += retrieveData[i].withdrawAmount;
           emit RetrieveTokensForWithdraw(
               retrieveData[i].withdrawAmount,
               retrieveData[i].connectorAddress,
               amount,
               amountAskedForWithdraw + amountAskedForWithdraw_temp
           );
       }
       amountAskedForWithdraw += amountAskedForWithdraw_temp;
       if (amountAskedForWithdraw_temp > neededAssets) {
           revert NoyaAccounting_INVALID_AMOUNT();
       }
   }

This code lacks mechanisms to check the price impact of retrieving large amounts from connectors, which could lead to unfavorable token retrieval rates.

Tools Used

Manual

Recommended Mitigation Steps

Integrating slippage control into the retrieveTokensForWithdraw function can be done by adding a check to ensure the amount of tokens retrieved is within an acceptable range of the expected amount. Here is a suggested implementation:

// Constants defining the maximum allowable slippage percentage (e.g., 1% slippage)
uint256 private constant MAX_SLIPPAGE = 100; // Representing 1%

function retrieveTokensForWithdraw(RetrieveData[] calldata retrieveData) public onlyManager nonReentrant {
    uint256 amountAskedForWithdraw_temp = 0;
    uint256 neededAssets = neededAssetsForWithdraw();
    for (uint256 i = 0; i < retrieveData.length; i++) {
        if (!registry.isAnActiveConnector(vaultId, retrieveData[i].connectorAddress)) {
            continue;
        }
        uint256 balanceBefore = baseToken.balanceOf(address(this));
        uint256 amount = IConnector(retrieveData[i].connectorAddress).sendTokensToTrustedAddress(
            address(baseToken), retrieveData[i].withdrawAmount, address(this), retrieveData[i].data
        );
        uint256 balanceAfter = baseToken.balanceOf(address(this));
        if (balanceBefore + amount > balanceAfter) revert NoyaAccounting_banalceAfterIsNotEnough();
        
        // Check for slippage
        uint256 expectedAmount = retrieveData[i].withdrawAmount;
        uint256 actualAmount = balanceAfter - balanceBefore;
        uint256 maxSlippageAmount = (expectedAmount * (10000 + MAX_SLIPPAGE)) / 10000;
        if (actualAmount < expectedAmount || actualAmount > maxSlippageAmount) {
            revert NoyaAccounting_SlippageTooHigh();
        }
        
        amountAskedForWithdraw_temp += actualAmount;
        emit RetrieveTokensForWithdraw(
            actualAmount,
            retrieveData[i].connectorAddress,
            amount,
            amountAskedForWithdraw + amountAskedForWithdraw_temp
            

Assessed type

Invalid Validation

[c4-pre-sort]

DadeKuma marked the issue as primary issue

[DadeKuma]

QA as these are deposits/withdrawals and not swaps

[c4-pre-sort]

DadeKuma marked the issue as insufficient quality report

[c4-judge]

gzeon-c4 marked the issue as satisfactory

[c4-judge]

gzeon-c4 marked the issue as selected for report

[gzeon-c4]

Deposit into a vault is implicitly a swap (deposit token to vault share).