Algem Diamond Refactoring

The pause() and unpause() functions of the LiquidStakingManager contract are not access controlled and can be used by an attacker to pause() and unpause() this contract at will.

The LiquidStakingManager contract has functions addManager() and removeManager() which require the DEFAULT_ADMIN_ROLE to execute. However, this role is not granted to the caller of the initialize() function, making them unusable.

LiquidStaking.sol

• LoC:22: The initialize() can be made external.
• LoC:30: The setMinStakeAmount() is not visible by LiquidStaking.sol.
• LoC:40-43: The initialize() function uses variable _dntUtil which used to be the 2nd argument of the constructor before the Diamond refactoring. However, now this argument was renamed to _utilName, but the code inside the function wasn’t updated accordingly.

LiquidStakingMain.sol

• LoC:194: The fix which removed the “()” from the use of the updateAll modifier in the withdraw() function was reverted.

• LoC:232: The fix which renamed the lastEtaTotalBalance to the correct lastEraTotalBalance was reverted.

• LoC:432: The _updateUserBalanceInUtility() uses the distr1_5 variable which is undeclared.

• LoC:493: The fix which renamed the message of the require() that had a typo was reverted.

LiquidStakingMigration.sol

• LoC:93: The migrateStorage() function was made public instead of external with no apparent need for such change.

NFTDistributor.sol

• LoC:432: This line contains the fix of the A6 issue from the main report which was introduced at commit 26c4028. However, it misses the semicolon (;) at the end of the line.

ArthswapAdapter.sol

• The WASTR address was mistakenly deleted when the fix for H1 of the main audit report was reverted in commit 25e78a.

ZenlinkAdapter.sol

• LoC:209: The pair.approve() and the pair.totalSupply() functions couldn’t be found in IZenlinkPair.sol.

The function findMedian() in the LiquidStakingMigration contract does not implement the median calculation correctly when the array is already in sorted order and is of even length. In this case, the result needs to be averaged between the two middle values.

LiquidStakingMigration::findMedian()

function findMedian(uint[] memory _arr) private pure returns (uint mean) {
    uint[] memory arr = _arr;
    uint len = arr.length;
    bool swapped = false;
    for (uint i; i < len - 1; i++) {
        for (uint j; j < len - i - 1; j++) {
            if (arr[j] > arr[j + 1]) {
                swapped = true;
                uint s = arr[j + 1];
                arr[j + 1] = arr[j];
                arr[j] = s;
            }
        }
        // Dedaub: The middle values need to be averaged
        //         here as well when len % 2 == 0
        if (!swapped) {
            return arr[len/2];
        }
    }
    if (len % 2 == 0)
        return (arr[len/2] + arr[len/2 - 1])/2;
    return arr[len/2];
}

The deleteSelector() function of the LiquidStakingManager contract, assigns the address(0) to the selectorToAddress mapping for the provided selector and then calls the private _deleteSelector() which erases the rest of the data related to this particular selector.

However, this function needs to retrieve the associated address with this selector in order to delete the rest data from the side of the address. Thus, since the address was deleted earlier, the function gets the address(0) as the to-be-deleted address and reverts due to underflow error when calculating the lastIndex of the list of the selectors.

As a result, the functions changeSelector() and changeSelector() that call deleteSelector() will eventually revert as well.

LiquidStakingManager::deleteSelector()

function deleteSelector(bytes4 selector) public onlyRole(MANAGER) {
    require(selectorToAddress[selector] != address(0),
        "The selector was not set");
    // Dedaub: Here the address of the selector is deleted before the call
    //         to _deleteSelector() which needs the original value
    selectorToAddress[selector] = address(0);

    _deleteSelector(selector);
}

LiquidStakingManager::_deleteSelector()

function _deleteSelector(bytes4 selector) private {

    // Dedaub: Here the function retrieves the address of the selector
    //         for deleting its related data, but it has already been
    //         zeroed at that point
    address addressFromSelector = selectorToAddress[selector];

    uint256 index = selectorIndex[addressFromSelector][selector];
    // Dedaub: Here the execution will revert due to an underflow error
    //         since the length of the array of the selectors for the
    //         address(0) is zero
    uint256 lastIndex = addressSelectors[addressFromSelector].length - 1;
    bytes4 lastSelector =
        addressSelectors[addressFromSelector][lastIndex];

    addressSelectors[addressFromSelector][index] =
        addressSelectors[addressFromSelector][lastIndex];
    selectorIndex[addressFromSelector][lastSelector] = index;

    addressSelectors[addressFromSelector].pop();

    if (addressSelectors[addressFromSelector].length == 0)
        _deleteAddress(addressFromSelector);
}

The current implementation of the Diamond pattern is not the standard implementation since it has some differences.

We raise here a warning for future maintainability for the reason that no use of hashed storage layout is made for ensuring no storage collisions.

However, in the current implementation the facets don’t have any storage variable of their own and they all inherit uniformly. This means that this issue doesn’t currently occur, but we raise this warning for future upgrades or facet additions that could introduce some storage variables or different inherited contract order which would cause a storage clash.

The LiquidStaking contract is the proxy contract which provides the entry point into the Diamond. It is called by other contracts and performs the required delegate calls to the other facet contracts. However, it also contains a setting() function which looks unrelated to the rest of its functionality. Possibly this may need to be moved to a facet instead.

The LiquidStakingManager contract has a contract variable named diamondAddress which is never initialized nor used.

The LiquidStakingMigration contract has a function getUserLpTokens() which uses the IPartnerHandler interface when calling calc(). However, no contract in the repository implements IPartnerHandler. Possibly the adapters need to implement this interface.

The code can be compiled with Solidity 0.8.4. Version 0.8.4, in particular, has some known bugs, which we do not believe affect the correctness of the contracts.