EigenLayer Contest - ihtishamsudo's results

[L-01]

deposit() does not emit an event, So it is difficult to track changes in the value of totalShares offchain

Proof Of Concept

function deposit(IERC20 token, uint256 amount)
        external
        virtual
        override
        onlyWhenNotPaused(PAUSED_DEPOSITS)
        onlyStrategyManager
        returns (uint256 newShares)
    {
        require(token == underlyingToken, "StrategyBase.deposit: Can only deposit underlyingToken");

        /**
         * @notice calculation of newShares *mirrors* `underlyingToShares(amount)`, but is different since the balance of `underlyingToken`
         * has already been increased due to the `strategyManager` transferring tokens to this strategy prior to calling this function
         */
        if (totalShares == 0) {
            newShares = amount;
        } else {
            uint256 priorTokenBalance = _tokenBalance() - amount;
            if (priorTokenBalance == 0) {
                newShares = amount;
            } else {
                newShares = (amount * totalShares) / priorTokenBalance;
            }
        }

        // checks to ensure correctness / avoid edge case where share rate can be massively inflated as a 'griefing' sort of attack
        require(newShares != 0, "StrategyBase.deposit: newShares cannot be zero");
        uint256 updatedTotalShares = totalShares + newShares;
        require(updatedTotalShares >= MIN_NONZERO_TOTAL_SHARES,
            "StrategyBase.deposit: updated totalShares amount would be nonzero but below MIN_NONZERO_TOTAL_SHARES");

        // update total share amount
        totalShares = updatedTotalShares;
        return newShares;
    }

Tools Used

Slither

Recommended Mitigation Steps

Emit events to notify about updated total shares.

L110:totalShares = updatedTotalShares;
//Should be updated with the emit  
emit TotalSharesUpdated(totalShares);

Emit The Event outside the deposit()

event TotalSharesUpdated(uint256 totalShares);

Reference

StrategyBase.sol#L110

Same Mitigation Steps as [L-01] in withdraw() at #L142

Reference

StrategyBase.sol#L142

[QA-01]

computePhase0BeaconBlockHeaderRoot() is never used and should be removed

Proof Of Concept

 function computePhase0BeaconBlockHeaderRoot(bytes32[NUM_BEACON_BLOCK_HEADER_FIELDS] calldata blockHeaderFields) internal pure returns(bytes32) {
        bytes32[] memory paddedHeaderFields = new bytes32[](2**BEACON_BLOCK_HEADER_FIELD_TREE_HEIGHT);
        
        for (uint256 i = 0; i < NUM_BEACON_BLOCK_HEADER_FIELDS; ++i) {
            paddedHeaderFields[i] = blockHeaderFields[i];
        }

        return Merkle.merkleizeSha256(paddedHeaderFields);
    }

Recommended Mitigation Steps

computePhase0BeaconBlockHeaderRoot() Should be removed to reduce code & to not make a code's review difficult

Reference

BeaconChainProofs.sol#L130-138

computePhase0BeaconStateRoot() Is Never Used & Should Be Removed

Reference

BeaconChainProofs.sol#L140-148

computePhase0ValidatorRoot() Is Never Used & Should Be Removed

Reference

BeaconChainProofs.sol#L150-158

computePhase0Eth1DataRoot() Is Never Used & Should Be Removed

Reference

BeaconChainProofs.sol#L160-168

[G-01]

Refactor sharesToUnderlyingView() the function that may result in saving gas

Proof Of Concept

function sharesToUnderlyingView(uint256 amountShares) public view virtual override returns (uint256) {
        if (totalShares == 0) {
            return amountShares;
        } else {
            return (_tokenBalance() * amountShares) / totalShares;
        }
    }

Recommended Mitigation Steps

function sharesToUnderlyingView(uint256 amountShares) public view virtual override returns (uint256) {
    if (totalShares == 0) {
        return amountShares;
    }
    
    uint256 tokenBalance = _tokenBalance();
    
    // Check for edge case where amountShares is equal to totalShares
    if (amountShares == totalShares) {
        return tokenBalance;
    }

    uint256 underlyingAmount = tokenBalance * amountShares;

    // The division can be moved outside the else block to save gas when totalShares is not equal to zero
    return underlyingAmount / totalShares;
}

Declared a tokenBalance variable to store the _tokenBalance() value once, instead of calling the function twice. Added a check for an edge case where amountShares is equal to totalShares, which avoids performing a multiplication and division that would return the same value. Moved the division operation outside the else block to avoid performing unnecessary calculations when totalShares is equal to zero.

Reference

StrategyBase.sol#L172-178

[G-02]

Use Import Aliases To Reduce Gas Cost

Proof Of Concept

import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "@openzeppelin-upgrades/contracts/proxy/utils/Initializable.sol";

Recommended Mitigation Steps

import { IERC20 as ERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import { SafeERC20 } from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import { Initializable } from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";

References

StrategyBase.sol#L6-8 StrategyManager.sol#L4-10 EigenPodManager.sol#L4-9 EigenPod.sol#L4-7 DelayedWithdrawalRouter.sol#L4-6