Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

M-09 MitigationConfirmed #26

Open
c4-bot-7 opened this issue Jun 6, 2024 · 1 comment
Open

M-09 MitigationConfirmed #26

c4-bot-7 opened this issue Jun 6, 2024 · 1 comment
Labels
mitigation-confirmed MR-M-09 satisfactory satisfies C4 submission criteria; eligible for awards

Comments

@c4-bot-7
Copy link
Contributor

c4-bot-7 commented Jun 6, 2024

Lines of code

Vulnerability details

See:

Finding Mitigation
M-09: Deposits will always revert if the amount being deposited is less than the bufferToFill value Pull Request

Navigating to M-09 from the previous contest we can see that there was an issue within the deposit function in the RestakeManager contract that enables users to deposit ERC20 whitelisted collateral tokens.

The crux of the issue was the fact that if the deposited amount is less than the buffer deficit, the entire deposit is used to fill the withdrawal buffer, leaving a zero amount for further operations which then leads to a problem when the execution tries to deposit this zero amount to the operator delegator which essentially reverts the transaction cause the OperatorDelegator's deposit function does not accept zero amounts if (address(tokenStrategyMapping[token]) == address(0x0) || tokenAmount == 0) revert InvalidZeroInput();. This issue then causes user deposits to fail if the deposit amount is less than the buffer deficit.

Now, to mitigate this issue, protocol used this pull request which sufficiently mitigates this issue considering the deposit function has now been modified to only approve and transfer the amount to the operator delegator if it is greater than zero, preventing the function from attempting to deposit zero amounts and thus ensuring successful deposits, i.e

        //  check if amount needs to be sent to operatorDelegator
+         if (_amount > 0) {
             // Approve the tokens to the operator delegator
             _collateralToken.safeApprove(address(operatorDelegator), _amount);

Now, whereas the changes are not correctly tagged with the pull request, I'm still tagging this as mitigation confirmed, considering from this pull request that was finalized to mitigate H-08 & M-09 from the previous contest we can see the complete implementation of the new changes

    function deposit(
        IERC20 _collateralToken,
        uint256 _amount,
        uint256 _referralId
    ) public nonReentrant notPaused {
        // Verify collateral token is in the list - call will revert if not found
        uint256 tokenIndex = getCollateralTokenIndex(_collateralToken);

        // Get the TVLs for each operator delegator and the total TVL
        (
            uint256[][] memory operatorDelegatorTokenTVLs,
            uint256[] memory operatorDelegatorTVLs,
            uint256 totalTVL
        ) = calculateTVLs();

        // Get the value of the collateral token being deposited
        uint256 collateralTokenValue = renzoOracle.lookupTokenValue(_collateralToken, _amount);

        // Enforce TVL limit if set, 0 means the check is not enabled
        if (maxDepositTVL != 0 && totalTVL + collateralTokenValue > maxDepositTVL) {
            revert MaxTVLReached();
        }

        // Enforce individual token TVL limit if set, 0 means the check is not enabled
        if (collateralTokenTvlLimits[_collateralToken] != 0) {
            // Track the current token's TVL
            uint256 currentTokenTVL = 0;

            // For each OD, add up the token TVLs
            uint256 odLength = operatorDelegatorTokenTVLs.length;
            for (uint256 i = 0; i < odLength; ) {
                currentTokenTVL += operatorDelegatorTokenTVLs[i][tokenIndex];
                unchecked {
                    ++i;
                }
            }

            // Check if it is over the limit
            if (currentTokenTVL + collateralTokenValue > collateralTokenTvlLimits[_collateralToken])
                revert MaxTokenTVLReached();
        }

        // Determine which operator delegator to use
        IOperatorDelegator operatorDelegator = chooseOperatorDelegatorForDeposit(
            operatorDelegatorTVLs,
            totalTVL
        );

        // Transfer the collateral token to this address
        _collateralToken.safeTransferFrom(msg.sender, address(this), _amount);

-        // Approve the tokens to the operator delegator
-        _collateralToken.safeApprove(address(operatorDelegator), _amount);

-        // Call deposit on the operator delegator
-        operatorDelegator.deposit(_collateralToken, _amount);

+        //  check if amount needs to be sent to operatorDelegator
+         if(_amount > 0) {
+             // Approve the tokens to the operator delegator
+             _collateralToken.safeApprove(address(operatorDelegator), _amount);

+             // Call deposit on the operator delegator
+             operatorDelegator.deposit(_collateralToken, _amount);
+         }
        // Calculate how much ezETH to mint
        uint256 ezETHToMint = renzoOracle.calculateMintAmount(
            totalTVL,
            collateralTokenValue,
            ezETH.totalSupply()
        );

        // Mint the ezETH
        ezETH.mint(msg.sender, ezETHToMint);

        // Emit the deposit event
        emit Deposit(msg.sender, _collateralToken, _amount, ezETHToMint, _referralId);
    }
@c4-judge
Copy link

c4-judge commented Jun 8, 2024

alcueca marked the issue as satisfactory

@c4-judge c4-judge added the satisfactory satisfies C4 submission criteria; eligible for awards label Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mitigation-confirmed MR-M-09 satisfactory satisfies C4 submission criteria; eligible for awards
Projects
None yet
Development

No branches or pull requests

2 participants