Algem

The NDistributor contract uses the users[] mapping to account for every user’s holdings among different utilities and DNT tokens. Before every mint, burn or transfer operation, the contract gets called in order to update the data of the users involved.

When a transfer happens, NDistributor::transferDnts() traverses the user’s utilities linearly to collect the funds required to fulfill the transfer. This way of collecting the funds from utilities can create issues when users have to get their funds back.

More specifically, suppose there are two users, User1 and User2. Suppose User1 stakes funds in dApp1, dApp2 and dApp3, and adds the corresponding DNT token liquidity to an Adapter. The adapter will book these funds against dApp1, dApp2 and dApp3.

Following this, User2 stakes funds in dApp4, dApp5 and dApp6 and adds the corresponding DNT token liquidity to the same Adapter. The adapter will then book these funds against dApp4, dApp5 and dApp6.

Now, if User2 decides to withdraw liquidity from the adapter, the adapter will return the funds by going through the dApps in order. As a result, when User2 receives the funds, these will be booked against dApp1, dApp2 and dApp3.

If User2 wants to unstake, he would expect the tokens he received to be attached to the utilities (dApps) he had originally staked on, but this may have changed as described above. So, if he tries to unstake providing his original dApps the function LiquidStaking::unstake() will check the balance booked against these apps using NDistributor::getUserDntBalanceInUtil(), find that the required amount is not sufficient (because it has been booked against a different utility), and revert, meaning that the user will be unable to unstake.

============================

Moreover, one more issue that arises, concerns the rewards that the dApps will receive (see here and here for more details upon reward distribution from the official docs).

dApps get their rewards depending on how many stakers have staked on that project. A user staking on a particular dApp means that he wants to support it. So, the problem here is that when a user decides to unstake from his dApps, if the above situation occurs, where his balances have been assigned to different dApps than the original ones, he may remove support from dApps he hasn’t even participated in. This can cause a loss in the potential rewards of the dApps.

Considering this on a larger scale, projects with dwindling support can remain active through the stake from other better-designed projects, since the stake will be first removed from those dApps that have been registered in the system first and have gained the trust of the people. This results in a greater issue which involves the whole Astar ecosystem and the way it operates and uses its reward programs (i.e. the Build2Earn program).

Below we develop a comprehensive example for demonstrating the issue:

==============================================

Execution Flow For Reproducing the Issue

• User1 calls ArthswapAdapter::addLiquidity() with all of his nASTR tokens

  • NASTR::safeTransferFrom() is called triggering the _beforeTokenTransfer() hook

    NASTR::_beforeTokenTransfer()

    function _beforeTokenTransfer(
        address from,
        address to,
        uint256 amount
    ) internal override(ERC20, ERC20Snapshot) whenNotPaused {
        super._beforeTokenTransfer(from, to, amount);
    
        if (isNote) {
            ...
        } else if (!isMultiTransfer) {
            (string[] memory utilities,
             uint256[] memory amounts
            ) = distributor.transferDnts(from, to, amount, "nASTR");
            nftDistr.multiTransferDnt(utilities, from, to, amounts);
        }
    }
    

  • Execution goes to if (!isMultiTransfer) and then NDistributor::transferDnts() is called

    • This function iterates over the user's utilities and tries to fill the order

      Note: The problem here is that the utilities get traversed in the order their were pushed in the array for each user

    NDistributor::transferDnts()

    function transferDnts(
        address _from, address _to,
        uint256 _amount, string memory _dnt
    ) external onlyRole(MANAGER)
      returns (string[] memory, uint256[] memory) {
        string[] memory _utilities =
            users[_from].dnt[_dnt].userUtils;
        uint256 l = _utilities.length;
        uint256[] memory amounts = new uint256[](l);
    
        for (uint256 i; i < l; i++) {
            // Dedaub: Utilities get traversed linearly ignoring that
            //         it will cause issues when receiving funds back
            uint256 senderBalance =
                users[_from].dnt[_dnt].dntInUtil[_utilities[i]];
            if (senderBalance > 0) {
                ...
                transferDnt(_from, _to,
                            takeFromUtility, _utilities[i], _dnt);
                ...
            }
        }
        revert("Not enough DNT");
    }
    
• All 3 dApps will be drained from User1 and transferred to the Adapter's acounting

  • So, after all 3 calls of NDistributor::transferDnt() are finished, the state will be as shown in the following table

    Table 2: User 1 Adds Liquidity
    User 1	users["user1"].userDnts = [] users["user1"].dnt["nASTR"].userUtils = [] users["user1"].dnt["nASTR"].dntInUtil["dApp1"] = 0 users["user1"].dnt["nASTR"].dntInUtil["dApp2"] = 0 users["user1"].dnt["nASTR"].dntInUtil["dApp3"] = 0
    User 2	users["user2"].userDnts = ["nASTR"] users["user2"].dnt["nASTR"].userUtils = [dApp4, dApp5, dApp6] users["user2"].dnt["nASTR"].dntInUtil["dApp4"] = 100 users["user2"].dnt["nASTR"].dntInUtil["dApp5"] = 100 users["user2"].dnt["nASTR"].dntInUtil["dApp6"] = 100
    Adapter	users["adapter"].userDnts = ["nASTR"] users["adapter"].dnt["nASTR"].userUtils = [dApp1, dApp2, dApp3] users["adapter"].dnt["nASTR"].dntInUtil["dApp1"] = 100 users["adapter"].dnt["nASTR"].dntInUtil["dApp2"] = 100 users["adapter"].dnt["nASTR"].dntInUtil["dApp3"] = 100

============================

• User2 calls ArthswapAdapter::addLiquidity() with all of his nASTR tokens

  • Following the previous execution flow the state will be as shown in in the following table

    Table 3: User 2 Adds Liquidity
    User 1	users["user1"].userDnts = [] users["user1"].dnt["nASTR"].userUtils = [] users["user1"].dnt["nASTR"].dntInUtil["dApp1"] = 0 users["user1"].dnt["nASTR"].dntInUtil["dApp2"] = 0 users["user1"].dnt["nASTR"].dntInUtil["dApp3"] = 0
    User 2	users["user2"].userDnts = [] users["user2"].dnt["nASTR"].userUtils = [] users["user2"].dnt["nASTR"].dntInUtil["dApp4"] = 0 users["user2"].dnt["nASTR"].dntInUtil["dApp5"] = 0 users["user2"].dnt["nASTR"].dntInUtil["dApp6"] = 0
    Adapter	users["adapter"].userDnts = ["nASTR"] users["adapter"].dnt["nASTR"].userUtils = [dApp1, dApp2, dApp3, dApp4, dApp5, dApp6] users["adapter"].dnt["nASTR"].dntInUtil["dApp1"] = 100 users["adapter"].dnt["nASTR"].dntInUtil["dApp2"] = 100 users["adapter"].dnt["nASTR"].dntInUtil["dApp3"] = 100 users["adapter"].dnt["nASTR"].dntInUtil["dApp4"] = 100 users["adapter"].dnt["nASTR"].dntInUtil["dApp5"] = 100 users["adapter"].dnt["nASTR"].dntInUtil["dApp6"] = 100

============================

• User2 calls ArthswapAdapter::removeLiquidity() for getting his nASTR tokens back
  • The execution here will be similar to the above described ones
  • The first 3 dApps in the Adapter's list will be drained since the iteration is linear over the utilities array

    • So, after all 3 calls of NDistributor::transferDnt() are finished, the state will be as shown in in the following table

      Table 4: User 2 Removes Liquidity
      User 1	users["user1"].userDnts = [] users["user1"].dnt["nASTR"].userUtils = [] users["user1"].dnt["nASTR"].dntInUtil["dApp1"] = 0 users["user1"].dnt["nASTR"].dntInUtil["dApp2"] = 0 users["user1"].dnt["nASTR"].dntInUtil["dApp3"] = 0
      User 2	users["user2"].userDnts = ["nASTR"] users["user2"].dnt["nASTR"].userUtils = [dApp1, dApp2, dApp3] users["user2"].dnt["nASTR"].dntInUtil["dApp1"] = 100 users["user2"].dnt["nASTR"].dntInUtil["dApp2"] = 100 users["user2"].dnt["nASTR"].dntInUtil["dApp3"] = 100 users["user2"].dnt["nASTR"].dntInUtil["dApp4"] = 0 users["user2"].dnt["nASTR"].dntInUtil["dApp5"] = 0 users["user2"].dnt["nASTR"].dntInUtil["dApp6"] = 0
      Adapter	users["adapter"].userDnts = ["nASTR"] users["adapter"].dnt["nASTR"].userUtils = [dApp4, dApp5, dApp6] users["adapter"].dnt["nASTR"].dntInUtil["dApp1"] = 0 users["adapter"].dnt["nASTR"].dntInUtil["dApp2"] = 0 users["adapter"].dnt["nASTR"].dntInUtil["dApp3"] = 0 users["adapter"].dnt["nASTR"].dntInUtil["dApp4"] = 100 users["adapter"].dnt["nASTR"].dntInUtil["dApp5"] = 100 users["adapter"].dnt["nASTR"].dntInUtil["dApp6"] = 100

============================

• User1 calls ArthswapAdapter::removeLiquidity() for getting his nASTR tokens back

  • Following the previous execution flow the state will be as shown in in the following table

    Table 5: User 1 Removes Liquidity
    User 1	users["user1"].userDnts = ["nASTR"] users["user1"].dnt["nASTR"].userUtils = [dApp4, dApp5, dApp6] users["user1"].dnt["nASTR"].dntInUtil["dApp1"] = 0 users["user1"].dnt["nASTR"].dntInUtil["dApp2"] = 0 users["user1"].dnt["nASTR"].dntInUtil["dApp3"] = 0 users["user1"].dnt["nASTR"].dntInUtil["dApp4"] = 100 users["user1"].dnt["nASTR"].dntInUtil["dApp5"] = 100 users["user1"].dnt["nASTR"].dntInUtil["dApp6"] = 100
    User 2	users["user2"].userDnts = ["nASTR"] users["user2"].dnt["nASTR"].userUtils = [dApp1, dApp2, dApp3] users["user2"].dnt["nASTR"].dntInUtil["dApp1"] = 100 users["user2"].dnt["nASTR"].dntInUtil["dApp2"] = 100 users["user2"].dnt["nASTR"].dntInUtil["dApp3"] = 100 users["user2"].dnt["nASTR"].dntInUtil["dApp4"] = 0 users["user2"].dnt["nASTR"].dntInUtil["dApp5"] = 0 users["user2"].dnt["nASTR"].dntInUtil["dApp6"] = 0
    Adapter	users["adapter"].userDnts = [] users["adapter"].dnt["nASTR"].userUtils = [] users["adapter"].dnt["nASTR"].dntInUtil["dApp1"] = 0 users["adapter"].dnt["nASTR"].dntInUtil["dApp2"] = 0 users["adapter"].dnt["nASTR"].dntInUtil["dApp3"] = 0 users["adapter"].dnt["nASTR"].dntInUtil["dApp4"] = 0 users["adapter"].dnt["nASTR"].dntInUtil["dApp5"] = 0 users["adapter"].dnt["nASTR"].dntInUtil["dApp6"] = 0

============================

The problem here is that the users would expect to unstake from different dApps, but providing a list of their original dApps will make the call revert. Moreover, even using the new ones to unstake from, the issue we described before about the breaking of the reward distribution of the dApps arises here as well.

In the LiquidStaking contract, the stake() function is payable, but the amount of ASTR sent into the function does not have to match the sum of the values in the _amounts parameter. Any funds in excess of this sum are not returned to the user but booked as revenue.

LiquidStaking::stake()

function stake(string[] memory _utilities, uint256[] memory _amounts)
    external payable
    checkArrays(_utilities, _amounts)
    updateAll
{
    // Dedaub: ASTR value sent can exceed the requested one for staking
    uint256 value = msg.value;

    uint256 l = _utilities.length;
    uint256 _stakeAmount;
    for (uint256 i; i < l; i++) {
        require(isActive[_utilities[i]], "Dapp not active");
        require(_amounts[i] >= minStakeAmount, "Not enough stake amount");

        _stakeAmount += _amounts[i];
    }
    ...
    totalBalance += _stakeAmount;
    // Dedaub: Here the difference isn't returned to the sender
    totalRevenue += value - _stakeAmount;
    ...
}

The Algem team has pointed out that this issue will be fixed in a future version of the protocol.

The _removeUtilityFromUser() function of the NDistributor contract removes a given utility from the user.

However, the users[] mapping holds the utilities in two different places:

• users[*].userUtilities
• users[*].dnt[*].userUtils

But, the first one holds all utilities for all DNT tokens used by the system while the second keeps the utilities for a specific DNT only. This means that the indexes between the two arrays will differ when the system uses more than one DNT token.

The function retrieves the index of the given utility from a global mapping which keeps the indexes according to the utils[*].userUtilities array. Then this index is used for deleting the utility from utils[*].dnt[*].userUtils as well which will fail since it will try to access an out-of-bounds position.

NDistributor::_removeUtilityFromUser()

function _removeUtilityFromUser(
    string memory _utility,
    string memory _dnt,
    address _user
) internal onlyRole(MANAGER) {

    // Dedaub: lastIdx can also result in out-of-bounds access
    uint lastIdx = users[_user].userUtilities.length - 1;

    //Dedaub: idx is retrieved from global mapping
    //         which is aligned with the users[\*].userUtilities
    uint idx = userUtitliesIdx[_user][_utility];

    userHasUtility[_user][_utility] = false;
    ...
    users[_user].userUtilities.pop();

    // Dedaub: users[\*].dnt[\*].userUtils[idx] will try to access
    //         out-of-bounds position when the system holds more
    //         than 1 DNT token
    users[_user].dnt[_dnt].userUtils[idx] =
        users[_user].dnt[_dnt].userUtils[lastIdx];
    users[_user].dnt[_dnt].userUtils.pop();
}

In the NDistributor contract, the function addUtilityToDisallowList() can be used to disallow utilities, but there is no function to allow a utility again in case of a mistake or a change of mind.

In the NDistributor contract removeManager() function moves the last manager to the index of the element which is supposed to be deleted but does not update managerIds mapping to point to the new index for the manager which has been moved. Due to this, further calls to removeManager() will start to cause unexpected results.

NDistributor::removeManager()

function removeManager(address _manager)
    public
    onlyRole(DEFAULT_ADMIN_ROLE)
{
    require(hasRole(MANAGER, _manager), "Address is not a manager");
    uint256 id = managerIds[_manager];

    // delete managers[id];
    managers[id] = managers[managers.length - 1];
    managers.pop();

    _revokeRole(MANAGER, _manager);

    // Dedaub: No update happens for the moved manager
    managerIds[_manager] = 0;
}

The NASTR contract’s constructor does not grant the DEFAULT_ADMIN_ROLE role to an address. But the revokeRole() function in the same contract has onlyRole(getRoleAdmin(role)) modifier, which by default is DEFAULT_ADMIN_ROLE. As a result the function will revert when called.

The NFTDistributor contract’s constructor does not grant the DEFAULT_ADMIN_ROLE role to an address. But the revokeRole(), addManager() and removeManager() functions can only be executed by an address with this role.

In the NDistributor contract, functions annotated with the dntInterface modifier will require the MANAGER role to call. However, not all functions annotated with this modifier require the MANAGER role to be invoked. Hence calls to these functions from a non-manager will cause them to revert. The functions affected are getUserDntBalance(), issueDnt() and transferDntContractOwnership().

In the NDistributor contract, the setup() function will not run if initialize2() has run, and vice versa, due to the shared isCalled contract variable. The functions affect different parts of the contract state and may need to be consolidated.

NDistributor::initialize2()

function initialize2() external {
    require(!isCalled, "Already called");
    isCalled = true;
    // commented for local tests
    // isUtility["LiquidStaking"] = true;
    isUtility["null"] = true;
    totalDnt["nASTR"] = totalDntInUtil["LiquidStaking"];
}

NDistributor::setup()

function setup() external onlyRole(MANAGER) {
    require(!isCalled, "Allready called");
    isCalled = true;
    isUtility["LiquidStaking"] = true;
    isUtility["null"] = true;
}

The notAllowContract modifier of the Arthswap contract, which is used in the main client facing code such as addLiqidity(), depositLP(), withdrawLP(), removeLiquidity(), addLP() and claim(), disables any possible automation going forward including from Algem’s own contracts. This approach is not recommended as it breaks the composability of contracts and may create issues in the future.

In the ZenlinkAdapter contract, the update modifier seems to be conflating rewards together. The call to farm.pendingRewards() returns an array of rewards, and the function accounts for them by summing them up. There may be a problem if the array has more than one element, for instance, if the farm starts returning more than one reward in the future.

ZenlinkAdapter::update()

modifier update() {
    ...
    for (uint i; i < rewardsArr.length;) {
        // Dedaub: Multiple rewards conflation
        unclaimedRewards += rewardsArr[i];
        unchecked { i++; }
    }
    ...
}

The changeMaxSupply() function of the Algem721 contract specifies that a manager can only increase or leave the totalSupply value as is. However, the manager can call the initialize2() function and set the totalSupply value to any value through this call instead.

Algem721::changeMaxSupply()

function changeMaxSupply(uint256 _maxSupply)
    external
    onlyRole(MANAGER_ROLE)
{
    // Dedaub: This require() disallows everyone from setting
    //         an arbitrary value to maxSupply
    require(_maxSupply >= totalSupply(), "Incorrect supply value");
    maxSupply = _maxSupply;
}

Algem721::initialize2()

function initialize2(
    address _nftDistr,
    string memory _utilName,
    uint256 _maxSupply
) external
  onlyRole(MANAGER_ROLE)
{
    nftDistr = NFTDistributor(_nftDistr);
    utilName = _utilName;

    // Dedaub: Manager can set an arbitrary value
    //         to maxSupply bypassing the checks
    maxSupply = _maxSupply;
}

The updateBalanceInAdapter() function in the AdaptersDistributor contract will underflow and revert if amountAfter < amountBefore. However, the if/else statement in this function suggests that there is a case where amountAfter < amountBefore is a possibility, because the else case actually computes AmountBefore - AmountAfter. However, execution will not get to the else case unless amountAfter = amountBefore, in which case the subtraction should just be replaced by 0. This suggests that the logic is not implemented correctly.

AdaptersDistributor::updateBalanceInAdapter()

function updateBalanceInAdapter(
    string memory _adapter,
    address user,
    uint256 amountAfter
) external onlyRole(ADAPTER) {
    uint256 amountBefore = adapters[_adapter].userAmount[user];

    if (amountBefore == amountAfter) return;

    totalAmount += amountAfter - amountBefore;
    userAmount[user] += amountAfter - amountBefore;
    adapters[_adapter].userAmount[user] += amountAfter - amountBefore;

    if (amountAfter > amountBefore) {
        nftDistr.transferDnt(utilName, address(0), user,
             amountAfter - amountBefore);
    } else {
        nftDistr.transferDnt(utilName, user, address(0),
             amountBefore - amountAfter);
    }
    liquidStaking.updateUserBalanceInAdapter(utilName, user);
}

The Pair used for the Zenlink adapter, has ordered the nASTR token first and ASTR token second. Even though the calculateRemoveLiquidity() uses the returned values in the correct order, the following functions do not, which can lead to incorrect results:

• Zenlink.sol::calc() assumes that nASTR is the second returned value of getReserves() while the first one is the correct one
• Zenlink.sol::getSecondAmount() uses the in the reverse order as well
• Zenlink.sol::totalReserves() has the same issue even though it just returns the sum of the reserves

See also A3 for some additional details and suggestions.

In NFTDistributor, mappings are using the utility names as keys to store and read data. However, when removing a utility (NFT) from the system the function removeUtility() doesn’t delete the data that was stored in the mappings for this NFT.

This may cause a conflict for a newly added NFT with the same name as the previously deleted one, since it will be mapped at the same location in memory, which may result in reading corrupted data and unexpected stored values.

NFTDistributor::removeUtility()

// Dedaub: This function leaves utils.[_utilname] location intact.
//         It just sets haveUtil[_utilName] to false

function removeUtility(string memory _utilName)
    public
    onlyRole(MANAGER)
{
    require(haveUtil[_utilName], "Utility not found");

    address utilAddress = utils[_utilName].contractAddress;

    haveUtil[_utilName] = false;
    uint256 _utilId = utilId[_utilName];
    utilsList[_utilId] = utilsList[utilsList.length - 1];
    utilId[utilsList[_utilId]] = _utilId;
    utilsList.pop();

    _revokeRole(TOKEN_CONTRACT, utilAddress);
}

The updateUserFee() function in NFTDistributor contract performs a similar operation like its private equivalent (_updateUserFee()), but it doesn’t update the users[*].defaultUserFee field. Only the users[*].eraDefaultUserFee gets updated.

NFTDistributor::updateUserFee()

function updateUserFee(
    address user,
    uint8 fee,
    uint256 era
) external onlyRole(MANAGER) {

    // Dedaub: Only the era's default fee gets updated
    if (users[user].eraDefaultUserFee[era] == 0)
        users[user].eraDefaultUserFee[era] = fee;
}

NFTDistributor::_updateUserFee()

function _updateUserFee(
    UserInfo storage user,
    uint8 fee,
    uint256 era
) private {
    user.defaultUserFee = fee;
    user.eraDefaultUserFee[era] = fee;
}

The setLiquidStaking() function in the NDistributor contract, assigns a new LiquidStaking contract to be used granting the MANAGER role as well for this new address. However, the MANAGER role isn’t get revoked from the old address.

We suggest revoking this role to ensure that the old contract won’t be able to access any of the distributor’s functionality.

NDistributor::setLiquidStaking()

function setLiquidStaking(address _liquidStaking)
    external
    onlyRole(DEFAULT_ADMIN_ROLE)
{
    require(_liquidStaking.isContract(),
        "_liquidStaking should be contract");
    //require(address(liquidStaking) == address(0), "Already set");
    liquidStaking = ILiquidStaking(_liquidStaking);
    _grantRole(MANAGER, _liquidStaking);
    emit SetLiquidStaking(_liquidStaking);
}

In several functions inside the LiquidStaking contract, the dapps[*].stakers[*].lastClaimedEra gets initialized with the value of the currentEra() + 1.

However, the functions eraShot() and migrationStorage() initialize this variable with the value of currentEra() not incremented by 1.

LiquidStaking::eraShot()

function eraShot(address _user) external onlyRole(MANAGER) {
    ...
    uint era = currentEra();
    ...
    if (usersShotsPerEra[_user][era].length == 0) {
        ...
        // Dedaub: lastClaimedEra gets the value of era instead of era + 1
        if (dapps["AdaptersUtility"].stakers[_user].lastClaimedEra == 0) {
            dapps["AdaptersUtility"].stakers[_user].lastClaimedEra = era;
        }
    }
    ...
}

============================

Moreover, there is a typo in the migrateStorage() function that can lead to incorrect results upon migrating.

Instead of getting the _era + 1 value from the staker’s data, there is a typo using the _era = 1 value for retrieving the data.

LiquidStaking::migrateStorage()

function migrateStorage(address _user) public onlyRole(MANAGER) {
    ...
    dapps[utilName].stakers[_user].eraBalance[_era + 1] =
        distr.getUserDntBalanceInUtil(_user, utilName, DNTname) -
        buffer[_user][_era + 1];
    // Dedaub: There is a typo using _era = 1 instead of _era + 1
    dapps[utilName].stakers[_user].isZeroBalance[_era + 1] =
        dapps[utilName]
          .stakers[_user]
          .eraBalance[_era = 1] == 0 ? true : false;
    dapps[utilName].stakers[_user].lastClaimedEra = _era;
}

The harvestRewards() function in the LiquidStaking contract is getting called in several places to sync the user’s rewards before each specific operation. For example, it is getting called whenever a user wants to initiate a claim or perform unstaking and upon syncing the rewards for a user by a functionality accessible only by the managers.

When users stake on a dApp for the first time, their corresponding data get initialized. More specifically, the dapps[*].stakers[*].lastClaimedEra gets the value of currentEra() + 1. This allows users to claim their rewards from at least the 2nd era from the one they staked on this dApp.

LiquidStaking::stake()

if (dapps[_utilities[i]].stakers[msg.sender].lastClaimedEra == 0)
    dapps[_utilities[i]].stakers[msg.sender].lastClaimedEra = _era + 1;

The harvestRewards() function calls the calcUserRewards() for calculating the rewards for all the eras between his lastClaimedEra and the global lastUpdated era. If the user tries to claim his rewards earlier, then this function will return no rewards and mark the user as not to-be-updated.

LiquidStaking::calcUserRewards()

function calcUserRewards(string memory _utility, address _user)
    private view returns(uint256[2] memory userData, uint8, uint256, bool)
{
    Staker storage user = dapps[_utility].stakers[_user];
    // Dedaub: When lastClaimedEra is >= lastUpdated era or the user
    //         hasn't staked on this dApp yet, empty data are returned
    if (user.lastClaimedEra >= lastUpdated || user.lastClaimedEra == 0)
        return (userData, 0, 0, false);

    ...
}

However, the harvestRewards() function will update the user’s lastClaimedEra field either way with the value of lastUpdateEra, which in particular can be the currentEra() value.

LiquidStaking::harvestRewards()

function harvestRewards(
    string memory _utility,
    address _user
) private {
    // calculate unclaimed user rewards
    (uint256[2] memory userData,
     uint8 newEraComission,
     uint256 userEraBalance,
     bool _updateUser
    ) = calcUserRewards(_utility, _user);
    if (_updateUser) {
        ...
    }
    // Dedaub: lastClaimedEra gets the value of lastUpdated which
    //         can allow users claim rewards earlier than supposed
    dapps[_utility].stakers[_user].lastClaimedEra = lastUpdated;
    ...
}

Assuming that the user’s lastClaimedEra was equal to currentEra() + 1, allowing him to claim at the 2nd era from the one he has staked on, after this update he will get the value of currentEra() and be able to claim rewards one era earlier.

============================

There are currently two ways to reproduce this issue and manipulate the function.

• The one is by having a manager call syncHarvest() for a specific user
• The other can be triggered by a user calling the claim() function where he can provide with his own dApp list and requested amounts to claim from.

Following, the second case a user can do the following to exploit the issue:

• Call claim() with utilities he hasn’t staked on yet but defining 0 requested amounts for them

LiquidStaking::claim()

function claim(string[] memory _utilities, uint256[] memory _amounts)
    public checkArrays(_utilities, _amounts)
    updateAll updateRewards(msg.sender, _utilities)
{
    _claim(_utilities, _amounts);
}

• The updateRewards modifier is called which iterates over the given dApp list and calls harvestRewards() for each one

LiquidStaking::updateRewards

modifier updateRewards(address _user, string[] memory _utilities) {
    uint256 l =_utilities.length;

    // harvest rewards for current balances
    for (uint256 i; i < l; i++) harvestRewards(_utilities[i], _user);
    _;
    // update balances in utils
    for (uint256 i; i < l; i++)
        _updateUserBalanceInUtility(_utilities[i], _user);
}

• User’s lastClaimedEra for the utilities that he hasn’t staked to yet is equal to 0

• Then this bug in harvestRewards() replaces this unitialized value with the value of lastUpdated even for these dApps

• After that, the execution goes to _claim() which iterates again over the dApp list and calculates the total amount of rewards that should be sent to the user

• Notes: In order to go through this function without reverting, the user needs to have staked to at least one dApp and provide an amount of rewards for claiming > 0, along with the rest dApps he hasn’t staked at which he wants to initialize for exploiting the issue

  LiquidStaking::_claim()

  function _claim(
      string[] memory _utilities, uint256[] memory _amounts
  ) private {
      uint256 l = _utilities.length;
      uint256 transferAmount;
  
      for (uint256 i; i < l; i++) {
          if (_amounts[i] > 0) {
              Dapp storage dapp = dapps[_utilities[i]];
               require(dapp.stakers[msg.sender].rewards
                   >= _amounts[i], "Not enough rewards!");
               require(rewardPool >= _amounts[i],
                   "Rewards pool drained");
              ...
              transferAmount += _amounts[i];
              ...
          }
      }
      require(transferAmount > 0, "Nothing to cliam");
  }
  

• Finishing the transfers, the function will normally return without reverting, which means that the data for the dApps the user hasn’t staked to have remained unchanged and corrupted

• Then, he can stake on the dApps for which he managed to manipulate the data before and be able to claim his rewards from these dApps much earlier than what he would originally be eligible to do

In LiquidStaking, the constructor calls setMinStakeAmount(10) and then manually assigns the value 10 to contract variable minStakeAmount immediately after. The second assignment is redundant.

LiquidStaking::constructor()

constructor(
    string memory _dntName,
    string memory _dntUtil,
    address _distrAddr)
{
    ...
    _grantRole(DEFAULT_ADMIN_ROLE, msg.sender);
    _grantRole(MANAGER, msg.sender);
    // Dedaub: minStakeAmount gets initialized here
    setMinStakeAmount(10);

    // Dedaub: Redundant assignment
    minStakeAmount = 10;
    ...
}

In the NDistributor contract, the changeOwner() function does not implement grant/claim ownership pattern, making it vulnerable to mistakes - for example, transferring ownership to the wrong address. The grant/claim pattern allows the contract owner to propose the address of a new owner, and the ownership does not change before the new owner accepts to become the new owner. Ownership transfer can be revoked during the intermediate period in case of a mistake.

The adapters use Pools for adding liquidity and each pool defines Pairs for the used tokens. However, the Pools sort the tokens by their addresses before creating the corresponding Pair. Then, the Pair keeps the reserves respectively to the sorted order of the tokens which may be different than the original provided one.

Assuming that, the returned values of the Pair::getReserves() shouldn’t be considered in line with the token order that was given upon adding liquidity. For example, PancakeLibrary has a getReserves() equivalent which takes the token order into account and adjusts the returned values accordingly.

PancakeLibrary::getReserves()

function getReserves(address factory, address tokenA, address tokenB)
    internal view
    returns(uint reserveA, uint reserveB)
{
    (address token0,) = sortTokens(tokenA, tokenB);
    pairFor(factory, tokenA, tokenB);
    (uint reserve0, uint reserve1,) =
        IPancakePair(pairFor(factory, tokenA, tokenB)).getReserves();
    (reserveA, reserveB) =
        tokenA == token0 ? (reserve0, reserve1) : (reserve1, reserve0);
}

Even though the current adapters are customized to get the Pair reserves in the correct order since the Pairs are already known, we suggest adding a more generic functionality similar to the getReserves() equivalent described before, which takes into account the order of the tokens and adjusts the returned values if needed, in order to be sure that adding more adapters in the future will not result to wrong handling of the reserves like in L3.

Moreover, we want to note that the MockZenlinkPair.sol contract incorrectly returns the ASTR first and nASTR second while the actual Pair returns the values in the reverse order.

The supportsInterface() function in NDistributor is redundant, since it just calls the super method of AccessControl, which is already inherited by the contract.

NDistributor::supportsInterface()

function supportsInterface(bytes4 interfaceId)
    public
    view
    override(AccessControl)
    returns (bool)
{
    return super.supportsInterface(interfaceId);
}

In the updatePoolRewards() of the ArthswapAdapter and the ZenlinkAdapter the “if totalStakedLP == 0” condition can be checked at the beginning of the function to save gas.

ArthswapAdapter::updatePoolRewards()

function updatePoolRewards() private {
    uint256 beforeArsw = arswToken.balanceOf(address(this));
    farm.harvest(pid, address(this));
    uint256 afterArsw = arswToken.balanceOf(address(this));
    uint256 receivedRewards = afterArsw > beforeArsw
        ? afterArsw - beforeArsw
        : 0;

    // Dedaub: This check can be moved at the beginning of the function
    if (totalStakedLp == 0) return;

    // increases accumulated rewards per 1 staked token
    accumulatedRewardsPerShare +=
        (receivedRewards * REWARDS_PRECISION) / totalStakedLp;
}

ZenlinkAdapter::updatePoolRewards()

function updatePoolRewards() private {
    uint256 beforeZlk = zlkToken.balanceOf(address(this));
    try farm.claim(pid) {}
    catch {
        emit NotClaimable();
    }
    uint256 afterZlk = zlkToken.balanceOf(address(this));
    uint256 receivedRewards = afterZlk > beforeZlk
        ? afterZlk - beforeZlk
        : 0;

    // Dedaub: This check can be moved at the beginning of the function
    if (totalStakedLp == 0) return;

    // increases accumulated rewards per 1 staked token
    accumulatedRewardsPerShare +=
        (receivedRewards * REWARDS_PRECISION) / totalStakedLp;
}

The functions issueDnt(), issueTransferDnt(), removeDnt() and removeTransferDnt() in NDistributor contract, there are no checks for the provided _amount parameter whether it is > 0 or not. However, when it is equal to 0, it is guaranteed that the execution will revert for certain execution flows, but this will happen after several function calls resulting in spending more gas than needed.

For example, let’s assume the issueTransferDnt() execution flow:

• An nASTR token transfer is initiated

• The _beforeTokenTransfer() execution will call:

• NDistributor::transferDnts() which will normally return even when the amount is 0

• NFTDistributor::multiTransferDnt() which will call:

• NFTDistributor::_transferDnt() which just returns when the given amount is 0

NFTDistributor::_transferDnt()

function _transferDnt(
    string memory utility,
    address from,
    address to,
    uint256 amount
) private {
    // Dedaub: It will return when the amount is 0
    if (amount == 0) return;
    ...
}

However, following the current implementation, some of these functions might only be called with non zero amounts since such checks happen before calling them, like for issueDnt() in LiquidStaking::stake(), but it might be worth keeping in mind for future development that gas could be saved in case this changes and these functions get called with zero amounts as well.

============================

The _removeNft() function in NFTDistributor contract, user’s NFT list is getting traversed in order to find the NFT that matches the given name to be removed. Since the names are strings, their hashes are compared against the given one. However, the hash of the provided NFT name could be calculated and cached out of the loop instead of being calculated again at each iteration.

NFTDistributor::_removeNft()

function _removeNft(
    string[] storage nftList,
    string memory utilName
) private returns (bool) {
    uint256 l = nftList.length;
    for (uint256 i; i < l; i++) {
        if (keccak256(abi.encodePacked(nftList[i])) ==
            // Dedaub: This hash can be calculated and cached
            //         outside of the loop for saving gas
            keccak256(abi.encodePacked(utilName)))
        {
            nftList[i] = nftList[l - 1];
            nftList.pop();
            break;
        }
    }
    return nftList.length == 0;
}

The withdrawRevenue() function in both the adapter contracts, can be called only by the owner, but there are no checks to ensure that the revenuePool has sufficient funds for the owner to withdraw the requested amount which can cause an underflow error.

ArthswapAdapter::withdrawRevenue()

ZenlinkAdapter::withdrawRevenue()

function withdrawRevenue(uint256 _amount) external onlyOwner {
    require(
        arswToken.balanceOf(address(this)) >= _amount,
        "Not enough ARSW/ZLK revenue"
    );
    require(_amount > 0, "Should be greater than zero");
    // Dedaub: Should be checked if revenuePool has enough funds
    revenuePool -= _amount;
    arswToken.safeTransfer(msg.sender, _amount);
}

The removeUtility() function in the AdaptersDistributorcontract should be renamed to removeAdapter(), since it deals with adapters not utilities.

AdaptersDistributor::removeUtility()

function removeUtility(string memory _utility)
    public onlyRole(MANAGER)
{ ... }

In the NASTR contract, the variable counter is internal and is being updated only upon minting or burning nASTR tokens. However, it is being only incremented and it is not used anywhere else inside the contract.

NASTR::_beforeTokenTransfer()

// Dedaub: Declared here with default visibility
uint256 counter;

function _beforeTokenTransfer(
    address from,
    address to,
    uint256 amount
) internal override(ERC20, ERC20Snapshot) whenNotPaused {
    ...
    if (isNote) {
        ...
        // Dedaub: It's only incremented and not used anywhere else
        counter++;
    } else if (!isMultiTransfer) {
        ...
    }
}

============================

In the LiquidStaking contract, the global variable stakers is only used by the view function getStakers(). In particular neither the variable nor the view function is used anywhere in the code.

LiquidStaking::getStakers()

function getStakers() external view returns (address[] memory) {
    return stakers;
}

The stakers array is an append only array, since it only gets elements upon calling stake() function or when addStaker() is called as well. However, there isn’t any remove function for this array.

============================

In the addAdapter() and removeUtility() functions in the AdaptersDistributor contract, it is implemented a logic which keeps track of the adapters that are inserted to or deleted from the system which in particular isn’t used anywhere else.

More specifically, the adaptersList array and adapterId mapping can be removed from these functions since they don’t get accessed from anywhere in the current implementation. They may be used in future updates, but we mention this here since removing them can save some gas.

AdaptersDistributor::addAdapter()

function addAdapter(address _contractAddress, string memory _utility)
    external onlyRole(MANAGER)
{
    require(_contractAddress != address(0), "Incorrect address");
    require(!haveAdapter[_utility], "Already have adapter");

    haveAdapter[_utility] = true;
    adapterId[_utility] = adaptersList.length;
    adaptersList.push(_utility);
    adapters[_utility].contractAddress = _contractAddress;

    _grantRole(ADAPTER, _contractAddress);
}

AdaptersDistributor::removeUtility()

function removeUtility(string memory _utility)
    public onlyRole(MANAGER)
{
    require(haveAdapter[_utility], "Adapter not found");
    address adapterAddress = adapters[_utility].contractAddress;

    haveAdapter[_utility] = false;
    uint256 _adapterId = adapterId[_utility];
    adaptersList[_adapterId] = adaptersList[adaptersList.length - 1];
    adapterId[adaptersList[_adapterId]] = _adapterId;
    adaptersList.pop();

    _revokeRole(ADAPTER, adapterAddress);
}

In both ArthswapAdapter and ZenlinkAdapter, removeLiquidity() emits an event with incorrect 2nd parameter.

As the event’s declaration states, the 2nd parameter was expected to be the amount of LP tokens the user wants to use for removing liquidity.

ArthswapAdapter::RemoveLiquidity

ZenlinkAdapter::RemoveLiquidity

event RemoveLiquidity(
    address indexed user,
    uint256 amountLP,
    uint256 indexed receivedASTR
);

However, the event is emitted with the amount of nASTR token that he received after removing liquidity.

ArthswapAdapter::removeLiquidity()

ZenlinkAdapter::removeLiquidity()

function removeLiquidity(uint256 _amount)
        public
        notAllowContract
        nonReentrant
{
    ...
    (uint amountToken, uint amountASTR) = pool.removeLiquidityETH(
        address(nToken),
        _amount,
        amountNASTRmin,
        amountASTRmin,
        address(this),
        block.timestamp + 1200
    );
    ...
    // Dedaub: Emission with amountToken when LP amount was expected
    emit RemoveLiquidity(msg.sender, amountToken, amountASTR);
}

The following functions could be made external instead of public, as they are not called by any of the contract’s functions:

The following variables could be made internal instead of public, as there are view functions to retrieve the data from:

The _claim() function in LiquidStaking contract, reverts when no amounts should be transferred with a mistyped message.

LiquidStaking::_claim()

function _claim(
    string[] memory _utilities,
    uint256[] memory _amounts
) private {
    uint256 l = _utilities.length;
    uint256 transferAmount;

    for (uint256 i; i < l; i++) { ... }
    // Dedaub: Typo in error message
    require(transferAmount > 0, "Nothing to cliam");
    ...
}

============================

The claimFromDapps() function in LiquidStaking contract, has a typo in the comments that describe the _claimFromDapp functionality. More specifically, lastEtaTotalBalance is used instead of the correct lastEraTotalBalance.

LiquidStaking::claimFromDapps()

function claimFromDapps(uint256 _era) private {
    ...
    // Dedaub: Typo in the comment
    /// this is due to the fact that <lastEtaTotalBalance> is updated at the moment of the previous era,
    ...
}

============================

The _addDntToUser() function in NFTDistributor contract, has a misleading comment for its parameters. It is described that the amount parameter holds the DNT that will be removed, while it should say that they are the tokens to be added.

LiquidStaking::_addDntToUser()

/// @notice function for redistribution of balances and fees for the user when adding DNT tokens to user.
/// @param utility => utility name.
/// @param to => address of the recipient.
// Dedaub: Should say `adding` instead of `removing`
/// @param amount => amount of removing DNT tokens;

/// @return userHasUtility => returns true if the user has the given utility.
function _addDntToUser(
    string memory utility, address to, uint256 amount
) private returns (bool) { ... }

The LiquidStaking contract has a lot of code that has been marked as unused and to-be-removed in future updates. However, some functions and variables, that are currently inside the active parts of the code, get initialized and they use some of these deprecated variables.

LiquidStaking::constructor()

constructor(
    string memory _dntName,
    string memory _dntUtil,
    address _distrAddr)
{
    ...
    // Dedaub: Remove decprecated variables
    lastStaked = era;
    lastClaimed = era;
    ...
}

LiquidStaking::initialize2()

function initialize2(
    address _nftDistr,
    address _adaptersDistr
) external onlyRole(MANAGER) {
    ...
    // Dedaub: Initializes the field with a deprecated global variable
    //         which is assigned as to be removed
    dapps[utilName].sum2unstake = sum2unstake;
    ...
}

LiquidStaking::migrateStorage()

function migrateStorage(address _user)
    public onlyRole(MANAGER)
{
    ...
    // Dedaub: buffer has been marked as unused
    dapps[utilName].stakers[_user].eraBalance[_era] =
        distr.getUserDntBalanceInUtil(_user, utilName, DNTname) -
            buffer[_user][_era];
    ...
    dapps[utilName].stakers[_user].eraBalance[_era + 1] =
        distr.getUserDntBalanceInUtil(_user, utilName, DNTname) -
            buffer[_user][_era + 1];
    ...
}

The listUserDnts(), listUserUtilitiesInDnt() and listUserDntInUtils() functions in the NDistributor contract, do not perform any checks for the _user argument if it is the zero address.

However, getUserDntBalanceInUtil() which is another view function similar to these ones, checks whether the _user parameter is the zero address or not and reverts if it does.

NDistributor::getUserDntBalanceInUtil()

function getUserDntBalanceInUtil(
    address _user,
    string memory _util,
    string memory _dnt
) public view returns (uint256) {
    require(_user != address(0), "Shouldn't be zero address");
    return users[_user].dnt[_dnt].dntInUtil[_util];
}

The updateAll modifier called by withdraw() and onlyDistributor modifier called by addStaker() are used with brackets while they don’t take any arguments.

Similar calls from other functions do not use brackets, so they can be removed here too for consistency.

LiquidStaking::withdraw()

function withdraw(uint _id) external updateAll() { ... }

LiquidStaking::addStaker()

function addStaker(
    address _addr, string memory _utility
) external onlyDistributor() { ... }

============================

In the constructor of the LiquidStaking contract the era variable gets the value by calling the DappStaking method, but there is a defined function that returns the same value and is widely used in the entire code, so it would make sense to keep it consistent here and use this function as well.

LiquidStaking::constructor()

constructor(
    string memory _dntName,
    string memory _dntUtil,
    address _distrAddr
) {
    ...
    // Dedaub: Here currentEra() could be used instead
    uint256 era = DAPPS_STAKING.read_current_era() - 1;
    ...
}

LiquidStaking::currentEra()

function currentEra() public view returns (uint) {
    return DAPPS_STAKING.read_current_era();
}

============================

In the functions calculateRemoveLiquidity(), calc() and getLpAmount() of the Zenlink adapter, in order to get the LP token total supply the lp.totalSupply() call is used. However, in the equivalent implementation of Arthswap adapter the pair.totalSupply() is used for the same reason. It would be preferable to keep the same calls between the two adapters for consistency since they return the same thing and since the LP address is expected to be the same as the Pair address as well.

ZenlinkAdapter::calculateRemoveLiquidity()

function calculateRemoveLiquidity(uint256 _amount)
    public view returns(uint256[] memory)
{
    (uint256 reservesNASTR, uint256 reservesASTR,) = pair.getReserves();
    uint256 totalLpSupply = lp.totalSupply();
    ...
}

ArthswapAdapter::calculateRemoveLiquidity()

function calculateRemoveLiquidity(uint256 _amount)
    public view returns (uint256[] memory)
{
    (uint256 reservesASTR, uint256 reservesNASTR,) = pair.getReserves();
    uint256 totalLpSupply = pair.totalSupply();
    ...
}

============================

In the initializer of the Algem721 contract, the _setupRole() is used for assigning the MANAGER role to msg.sender. However, the _setupRole() has been marked as deprecated and we suggest using the grantRole() equivalent since it has been used for any other role grants.

Algem721::initialize()

function initialize(
    string memory name,
    string memory symbol,
    string memory baseTokenURI
) public override initializer {
    super.initialize(name, symbol, baseTokenURI);
    _baseTokenURI = baseTokenURI;
    // Dedaub: `_grantRole()` can be used for consistency
    _setupRole(MANAGER_ROLE, _msgSender());
}

The listUserUtilitiesInDnt() and getUserUtilsInDnt() functions in the NDistributor contract, do the exact same thing, returning the user’s util list.

NDistributor::listUserUtilitiesInDnt()

function listUserUtilitiesInDnt(
    address _user, string memory _dnt
) public view returns (string[] memory) {
    return users[_user].dnt[_dnt].userUtils;
}