eta

medium

Unhandled Return Data in `Delegatecall`, Memory Optimization, Unused Parameter and Unsafe Code Cast

Summary

This code review highlights four issues for improvement: 1)Unhandled Return Data in Diamond:: fallback: The Diamond::fallback function's external call doesn't handle return data of delegatecall, potentially exposing it to gas griefing attacks. 2)Optimize Logic in Setter Function: To enhance change tracking and potentially reduce memory usage, record both old and new values for critical parameter changes within setter functions in LibDiamond.sol and LibUbiquityPool.sol, avoiding unnecessary caching. 3)Unused Parameter in LibDiamond::removeFunctions: The LibDiamond::removeFunctions function has an unnecessary parameter, _facetAddress, which is unused in its logic to check for address(0), with a notice to check if function does not exist then do nothing and return which is more relevant to the _functionSelectors requirement. 4)Code Cast from Unit256 to Uint112 and back to Uint256: The code casts a uint256 value from IMetaPool::balances to a smaller uint112 type and then immediately to the uint256 type in LibTWAPOracle::setPool.

Vulnerability Detail

1.Unhandled Return Data in Diamond:: fallback

EVM Return Data Handling: When a contract calls another contract (external calls), the EVM allocates memory to store the called contract's return data, including a boolean success value indicating the call outcome.

Unhandled Return Data: In the code snippet, let result := delegatecall(gas(), facet, 0, calldatasize(), 0, 0) performs an external call without specifying any output buffer (out) or its size (outsize). This means the return data is discarded, including the success value.

Potential Gas Griefing: Malicious external contracts could exploit this by intentionally returning large amounts of data, consuming more gas than expected. This could lead to unexpected gas costs for the calling contract or even prevent it from completing the transaction, potentially disrupting its functionality.

Diamond::fallback


    fallback() external payable {
...
        assembly {
            calldatacopy(0, 0, calldatasize())
@audit      let result := delegatecall(gas(), facet, 0, calldatasize(), 0, 0)
            returndatacopy(0, 0, returndatasize())
            switch result
            case 0 {
                revert(0, returndatasize())
            }
            default {
                return(0, returndatasize())
            }
        }
}

2.Improve Logic in Setter Function in LibDiamond.sol and LibUbiquityPool.sol

Record both old and new values: Directly within setter functions, it is recommended to record both its previous and updated values of critical parameters. This creates a detailed change history, enabling accurate tracking of parameter modifications, informed analysis of change patterns and potential rollback to a previous state if required.

Eliminate unnecessary caching: If the old value is only needed for immediate actions (e.g., event emission), access it directly within the setter to streamline code and potentially reduce memory overhead. This streamlines code, potentially reduces memory overhead, and improve code readability.

LibDiamond::setContractOwner


    function setContractOwner(address _newOwner) internal {
        DiamondStorage storage ds = diamondStorage();
+       emit OwnershipTransferred(ds.contractOwner, _newOwner);
_       address previousOwner = ds.contractOwner;
        ds.contractOwner = _newOwner;
_       emit OwnershipTransferred(previousOwner, _newOwner);
}

All other instances entailed:

LibUbiquityPool::updateChainLinkCollateralPrice


function updateChainLinkCollateralPrice(uint256 collateralIndex) internal {
...
+       emit CollateralPriceSet(collateralIndex, poolStorage.collateralPrices[collateralIndex], price);
        poolStorage.collateralPrices[collateralIndex] = price;

-       emit CollateralPriceSet(collateralIndex, price);
}

LibUbiquityPool::setFees


    function setFees(
        uint256 collateralIndex,
        uint256 newMintFee,
        uint256 newRedeemFee
    ) internal {
        UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();

+        emit FeesSet(collateralIndex,poolStorage.mintingFee[collateralIndex],  newMintFee, poolStorage.redemptionFee[collateralIndex], newRedeemFee);

        poolStorage.mintingFee[collateralIndex] = newMintFee;
        poolStorage.redemptionFee[collateralIndex] = newRedeemFee;

-       emit FeesSet(collateralIndex, newMintFee, newRedeemFee);
}

LibUbiquityPool::setPoolCeiling


    function setPoolCeiling(
        uint256 collateralIndex,
        uint256 newCeiling
    ) internal {
        UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();

+       emit PoolCeilingSet(poolStorage.poolCeilings[collateralIndex], newCeiling);

        poolStorage.poolCeilings[collateralIndex] = newCeiling;

-       emit PoolCeilingSet(collateralIndex, newCeiling);
}

LibUbiquityPool::setPriceThresholds


    function setPriceThresholds(
        uint256 newMintPriceThreshold,
        uint256 newRedeemPriceThreshold
    ) internal {
        UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();

+       emit PriceThresholdsSet(poolStorage.mintPriceThreshold, newMintPriceThreshold, poolStorage.redeemPriceThreshold, newRedeemPriceThreshold);

        poolStorage.mintPriceThreshold = newMintPriceThreshold;
        poolStorage.redeemPriceThreshold = newRedeemPriceThreshold;

-       emit PriceThresholdsSet(newMintPriceThreshold, newRedeemPriceThreshold);
}

LibUbiquityPool::setRedemptionDelayBlocks


    function setRedemptionDelayBlocks(
        uint256 newRedemptionDelayBlocks
    ) internal {
        UbiquityPoolStorage storage poolStorage = ubiquityPoolStorage();

+       emit RedemptionDelayBlocksSet(poolStorage.redemptionDelayBlocks, newRedemptionDelayBlocks);

        poolStorage.redemptionDelayBlocks = newRedemptionDelayBlocks;

-       emit RedemptionDelayBlocksSet(newRedemptionDelayBlocks);
}

3.Unused Parameter in LibDiamond::removeFunctions

TheremoveFunctions function has an unused parameter, _facetAddress, which is passed to it but never utilized within the logic of both the removeFunctions and the removeFunction functions.

This parameter is only checked in a require statement that ensures its value is address(0) as mentioned by a notice. However, this notice of // if function does not exist then do nothing and return is more logically relevant to the _functionSelectors requirement.

LibDiamond::removeFunctions


    function removeFunctions(
-       address _facetAddress,
        bytes4[] memory _functionSelectors
) internal {
+       // if function does not exist then do nothing and return
        require(
            _functionSelectors.length > 0,
            "LibDiamondCut: No selectors in facet to cut"
        );
        DiamondStorage storage ds = diamondStorage();
 -      // if function does not exist then do nothing and return
 -      require(
 -          _facetAddress == address(0),
 -          "LibDiamondCut: Remove facet address must be address(0)"
 -      );
        for (
            uint256 selectorIndex;
            selectorIndex < _functionSelectors.length;
            selectorIndex++
        ) {
            bytes4 selector = _functionSelectors[selectorIndex];
            address oldFacetAddress = ds
                .selectorToFacetAndPosition[selector]
                .facetAddress;
            removeFunction(ds, oldFacetAddress, selector);
        }
}

4.Consider to Use SafeCast for Casting

Casting from larger types to smaller ones can potentially lead to overflows and thus unexpected behavior. When the uint256 value in the IMetaPool::balances gets cast to uint112 in the LibTWAPOracle::setPool, it could end up as an entirely different, much smaller number due to the reduction in bits.

OpenZeppelin's SafeCast library provides functions for safe type conversions, throwing an error whenever an overflow would occur. It is generally recommended to use SafeCast or similar protective measures when performing type conversions to ensure the accuracy of your computations and the security of your contracts.

LibTWAPOracle::setPool


function setPool(address _pool, address _curve3CRVToken1) internal {
   ...
-      uint256 _reserve0 = uint112(IMetaPool(_pool).balances(0));
-      uint256 _reserve1 = uint112(IMetaPool(_pool).balances(1));
+      uint256 _reserve0 = SafeCast.toUint112 (IMetaPool(_pool).balances(0));
+      uint256 _reserve1 = SafeCast.toUint112(IMetaPool(_pool).balances(1));
       ...
}

IMetaPool::balances


function balances(uint256 arg0) external view returns (uint256);

Impact

Unhandled Return Data and Potential Gas Griefing: By ignoring the return data from external calls, this code opens the door to gas griefing, so that malicious contracts can force high gas usage, potentially preventing completion or causing unexpected financial losses.

Inaccurate Tracing and Analysis: Without capturing both old and new values, precise tracking of parameter changesis impossible, hindering trend analysis and issue diagnosis.

Unused Parameter: The presence of an unused parameter introduces unnecessary complexity and opens potential security vulnerabilities for malicious actors to exploit.

Overflow Risk in Type Casting: Overflows can result in unexpected and incorrect calculations, potentially leading to financial losses, contract malfunctions, or security vulnerabilities.

Code Snippet

1) Diamond::fallback

2) LibDiamond::setContractOwner LibUbiquityPool::updateChainLinkCollateralPrice LibUbiquityPool::setFees LibUbiquityPool::setPoolCeiling LibUbiquityPool::setPriceThresholds LibUbiquityPool::setRedemptionDelayBlocks

3) LibDiamond::removeFunctions

4) LibTWAPOracle::setPool IMetaPool::balances

Tool used

Manual Review

Recommendation

Use Safe Delecatecall Pattern: Employ the delegatecall function with appropriate out and outsize parameters to capture the return data. This allows for proper handling of the success value and prevents unexpected gas consumption.

Prioritize Change Tracking and Memory Efficiency: Capture both old and new values directly within setter functions for critical parameters to establish a detailed change history. Avoid unnecessary caching by directly accessing old values within the functions when feasible, which potentially reduces memory overhead and enhances code readability.

Remove _facetAddress Parameter: Eliminate the _facetAddress parameter from the removeFunctions function to eliminate potential confusion, and reduces the risk of unintended vulnerabilities related to unused parameters.

Use Safe Casting Libraries: Employ OpenZeppelin's SafeCast library or similar tools for safe type conversions. If possible, use larger data types (e.g., uint256) throughout the contract to avoid the need for casting and reduce the risk of overflows.

sherlock-audit / 2023-12-ubiquity-judging

eta - Unhandled Return Data in `Delegatecall`, Memory Optimization, Unused Parameter and Unsafe Code Cast #152

Unhandled Return Data in `Delegatecall`, Memory Optimization, Unused Parameter and Unsafe Code Cast

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

sherlock-audit / 2023-12-ubiquity-judging

eta - Unhandled Return Data in `Delegatecall`, Memory Optimization, Unused Parameter and Unsafe Code Cast #152

Unhandled Return Data in Delegatecall, Memory Optimization, Unused Parameter and Unsafe Code Cast

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation

Unhandled Return Data in `Delegatecall`, Memory Optimization, Unused Parameter and Unsafe Code Cast