Skip to main content
Dedaub

Algem

Smart Contract Security Assessment

November 18, 2022

Algem

SUMMARY

ID
Description
STATUS

ABSTRACT

Dedaub was commissioned to perform a security audit of the Algem protocol.

Algem protocol is a DeFi dApp built on top of Astar Network. One of the features offered by the Algem protocol is the concept of liquid staking. Through this feature, users can stake their ASTR tokens through the Astar Network’s DappStaking programme, and also receive nASTR tokens in a 1:1 ratio in return. These nASTR tokens, which maintain a peg with ASTR tokens through arbitrage, can then be used to participate in other DeFi protocols and earn extra yield. In this way, the user remains liquid whilst obtaining the benefits of staking his or her ASTR tokens.

This audit report covers the contracts of the repository ilkatel/liquidStaking_audit of the Algem protocol, up to commit hash 0d1aa2e799ee03b116283d4a7a03ab5b9df57a48.

The codebase contains a number of contracts with complex business logic, such as the LiquidStaking, NDistributor and NFTDistributor contracts. A substantial number of contract variables are used, and the coupling present between functions requires the shared state to be properly maintained at all times. In addition a number of contracts need to communicate with one another in order to keep the overall state of the protocol in sync.

An effort was made to verify the workflows and interaction between these contracts with the Algem team. However, due to the large state space and complex business logic, the development of an extensive test suite with high code coverage is being highly recommended, especially for the aforementioned contracts. This includes testing for scenarios involving multiple users stressing workflows in a sequence, due to the presence of temporal dependencies between subsequent calls.

Two auditors worked on the codebase for 14 days on the following contracts:

contracts/audit/
  • AdaptersDistributor.sol
  • Algem721.sol
  • ArthswapAdapter.sol
  • LiquidStaking.sol
  • NASTR.sol
  • NDistributor.sol
  • NFTDistributor.sol
  • ZenlinkAdapter.sol
  • interfaces/
    • DappsStaking.sol
    • IAdaptersDistributor.sol
    • IDNT.sol
    • IDNTMultiTransfer.sol
    • ILiquidStaking.sol
    • IMasterChef.sol
    • INDistributor.sol
    • INFTDistributor.sol
    • IPancakePair.sol
    • IPancakeRouter01.sol
    • IPartnerHandler.sol

SETTING & CAVEATS

The audit’s main target is security threats, i.e., what the community understanding would likely call "hacking", rather than the regular use of the protocol. Functional correctness (i.e. issues in "regular use") is a secondary consideration. Typically it can only be covered if we are provided with unambiguous (i.e. full-detail) specifications of what is the expected, correct behaviour. In terms of functional correctness, we often trusted the code’s calculations and interactions, in the absence of any other specification. Functional correctness relative to low-level calculations (including units, scaling and quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.


VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues affecting the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:

Category
Description
CRITICAL
Can be profitably exploited by any knowledgeable third-party attacker to drain a portion of the system’s or users’ funds OR the contract does not function as intended and severe loss of funds may result.
HIGH
Third-party attackers or faulty functionality may block the system or cause the system or users to lose funds. Important system invariants can be violated.
MEDIUM
Examples:
  • User or system funds can be lost when third-party systems misbehave.
  • DoS, under specific conditions.
  • Part of the functionality becomes unusable due to a programming error.
LOW
Examples:
  • Breaking important system invariants but without apparent consequences.
  • Buggy functionality for trusted users where a workaround exists.
  • Security issues which may manifest when the system evolves.

Issue resolution includes “dismissed” or “acknowledged” but no action taken, by the client, or “resolved”, per the auditors.


CRITICAL SEVERITY

[No critical severity issues]


HIGH SEVERITY

H1

Users may be able to unstake from dApps they haven’t staked at all altering the rewards of the dApps

H1HIGH

Users may be able to unstake from dApps they haven’t staked at all altering the rewards of the dApps
acknowledged
The Algem team has pointed out that this issue will be fixed in a future version of the protocol.

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:

Initial State
Table 1: User 1 & 2 Have Staked Into Different dApps
User 1

Balance: 300 nASTR

dApps: dApp1, dApp2, dApp3

users["user1"].userDnts = ["nASTR"]

users["user1"].dnt["nASTR"].userUtils =

[dApp1, dApp2, dApp3]

users["user1"].dnt["nASTR"].dntInUtil["dApp1"] = 100

users["user1"].dnt["nASTR"].dntInUtil["dApp2"] = 100

users["user1"].dnt["nASTR"].dntInUtil["dApp3"] = 100

User 2

Balance: 300 nASTR

dApps: dApp4, dApp5, dApp6

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 = []

users["adapter"].dnt["nASTR"].userUtils = []

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

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.

H2

Users can mistakenly lose funds when calling the LiquidStaking::stake() function

H2HIGH

Users can mistakenly lose funds when calling the LiquidStaking::stake() function
resolved

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;
...
}

H3

Use of more than one DNT tokens will cause a crash to the NDistributor contract

H3HIGH

Use of more than one DNT tokens will cause a crash to the NDistributor contract
acknowledged

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();
}


MEDIUM SEVERITY

M1

NDistributor cannot allow utilities again once they are disallowed

M1MEDIUM

NDistributor cannot allow utilities again once they are disallowed
resolved

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.

M2

Faulty NDistributor contract removeManager() function implementation

M2MEDIUM

Faulty NDistributor contract removeManager() function implementation
resolved

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;
}

M3

DEFAULT_ADMIN_ROLE not assigned in NASTR contract’s constructor

M3MEDIUM

DEFAULT_ADMIN_ROLE not assigned in NASTR contract’s constructor
resolved

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.

M4

DEFAULT_ADMIN_ROLE not assigned in NFTDistributor contract’s constructor

M4MEDIUM

DEFAULT_ADMIN_ROLE not assigned in NFTDistributor contract’s constructor
resolved

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.

M5

In NDistributor, functions with dntInterface modifier which do not require MANAGER role will revert

M5MEDIUM

In NDistributor, functions with dntInterface modifier which do not require MANAGER role will revert
resolved

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().

M6

Functions initialize2() and setup() are contradictory in NDistributor

M6MEDIUM

Functions initialize2() and setup() are contradictory in NDistributor
resolved

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;
}


LOW SEVERITY

L1

Arthswap notAllowContract modifier disables all automations going forward

L1LOW

Arthswap notAllowContract modifier disables all automations going forward
resolved

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.

L2

ZenlinkAdapter contract conflating multiple rewards into a single value

L2LOW

ZenlinkAdapter contract conflating multiple rewards into a single value
dismissed

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++; }
}
...
}

L3

Manager can bypass checks when setting totalSupply in Algem721 contract

L3LOW

Manager can bypass checks when setting totalSupply in Algem721 contract
resolved

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;
}

L4

Possible incorrect implementation in AdaptersDistributor’s updateBalanceInAdapter() function

L4LOW

Possible incorrect implementation in AdaptersDistributor’s updateBalanceInAdapter() function
resolved

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);
}

L5

Incorrect use of the Pair::getReserves() returned values

L5LOW

Incorrect use of the Pair::getReserves() returned values
resolved

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.

L6

Possible use of corrupt data when adding a new NFT with the same name as a deleted one

L6LOW

Possible use of corrupt data when adding a new NFT with the same name as a deleted one
open

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);
}

L7

Possible incorrect update of user’s fee in NFTDistributor

L7LOW

Possible incorrect update of user’s fee in NFTDistributor
dismissed

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;
}

L8

Changing LiquidStaking contract in NDistributor doesn’t revoke the MANAGER role from the old one

L8LOW

Changing LiquidStaking contract in NDistributor doesn’t revoke the MANAGER role from the old one
resolved

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);
}

L9

Incorrect initialization of lastClaimedEra in some functions

L9LOW

Incorrect initialization of lastClaimedEra in some functions
resolved

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;
}

L10

Incorrect update of user’s lastClaimedEra value, upon user reward harvesting, can result in issues when claiming

L10LOW

Incorrect update of user’s lastClaimedEra value, upon user reward harvesting, can result in issues when claiming
open

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



OTHER / ADVISORY ISSUES

This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.

A1

Redundant assignment in LiquidStaking constructor

A1ADVISORY

Redundant assignment in LiquidStaking constructor
resolved

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;
...
}

A2

NDistributor changeOwner() function does not implement grant/claim ownership pattern

A2ADVISORY

NDistributor changeOwner() function does not implement grant/claim ownership pattern
resolved

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.

A3

Possible misuse of the Pair::getReserves() returned values

A3ADVISORY

Possible misuse of the Pair::getReserves() returned values
resolved

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.

A4

Redundant supportsInterface function in NDistributor

A4ADVISORY

Redundant supportsInterface function in NDistributor
resolved

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);
}

A5

Possible gas optimisation in updatePoolRewards() function of both adapters

A5ADVISORY

Possible gas optimisation in updatePoolRewards() function of both adapters
resolved

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;
}

A6

Other gas optimizations

A6ADVISORY

Other gas optimizations
resolved

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;
}

A7

Trying to withdraw revenue in adapters may revert when there are no enough funds in the revenue pool

A7ADVISORY

Trying to withdraw revenue in adapters may revert when there are no enough funds in the revenue pool
resolved

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);
}

A8

Misleading name in AdaptersDistributor contract

A8ADVISORY

Misleading name in AdaptersDistributor contract
resolved

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)
{ ... }

A9

Updating variables that are not used anywhere

A9ADVISORY

Updating variables that are not used anywhere
resolved

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);
}

A10

Wrong event emission

A10ADVISORY

Wrong event emission
resolved

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);
}

A11

Functions could be made external

A11ADVISORY

Functions could be made external
resolved

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

  • LiquidStaking.sol

  • claim()

  • getUserRewards()

  • migrateStorage()

  • AdapterDistributor.sol

  • removeUtility()

  • ZenlinkAdapter.sol

  • pendingRewards()

  • gaugeBalances()

  • totalReserves()

  • getInfo()

  • getLpAmount()

  • NDistributor.sol

  • addUtilityToDisallowList()

  • setUtilityStatus()

  • setDntStatus()

  • listUserDnts()

  • getUserDntBalanceInUtil()

  • getUserUtilsInDnt()

  • getUserDntBalance()

A12

The scope of some variables can be restricted to internal

A12ADVISORY

The scope of some variables can be restricted to internal
resolved

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

  • LiquidStaking.sol

  • stakers

  • totalUserRewards

  • withdrawals

Corresponding View Functions
  • getStakers()
  • getUserRewards()
  • getUserWithdrawals()

A13

Mistyped comments and error messages

A13ADVISORY

Mistyped comments and error messages
resolved

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) { ... }

A14

Use of variables marked as to-be-removed

A14ADVISORY

Use of variables marked as to-be-removed
dismissed

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];
...
}

A15

User address is not checked for zero address

A15ADVISORY

User address is not checked for zero address
resolved

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];
}

A16

Code consistency suggestions

A16ADVISORY

Code consistency suggestions
resolved

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());
}

A17

Duplicate function

A17ADVISORY

Duplicate function
resolved

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;
}

NDistributor::getUserUtilsInDnt()

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

A18

NFTDistributor::multiTransferDnt() allows the AdaptersDistributor contract to call it

A18ADVISORY

NFTDistributor::multiTransferDnt() allows the AdaptersDistributor contract to call it
resolved

The multiTransferDnt() function in the NFTDistributor contract, allows the NASTR token contract along with the AdaptersDistributor contract to call it.

NFTDistributor::multiTransferDnt()

function multiTransferDnt(
string[] memory utilities,
address from, address to,
uint256[] memory amounts
) external globalUpdate {
require(msg.sender == address(nAstr) ||
msg.sender == adaptersDistributor, "Not access");

uint256 l = utilities.length;
for (uint256 i; i < l; i++) {
_transferDnt(utilities[i], from, to, amounts[i]);
}
}

However, the AdaptersDistributor doesn’t contain any relevant calls in the current implementation.

We would like to mention here that if in a future implementation the AdaptersDistributor contract make calls to this function, the length of the arrays should be checked for compatibility before making the call, similar to what happens in NASTR::transferFromUtilities().

A19

NDistributor::transferDnt implementations have different check handling for the given amount argument

A19ADVISORY

NDistributor::*transferDnt* implementations have different check handling for the given amount argument
dismissed

The transferDnt() function in the NDistributor contract, contains a require which checks whether the given amount is > 0.

NFTDistributor::transferDnt()

function transferDnt(
string memory utility,
address from, address to,
uint256 amount
) external globalUpdate {
require(msg.sender == address(nAstr) ||
msg.sender == adaptersDistributor, "Not access");
require(amount > 0, "Incorrect amount");
_transferDnt(utility, from, to, amount);
}

However, multiTransferDnt() doesn't make any relevant checks.

NFTDistributor::multiTransferDnt()

function multiTransferDnt(
string[] memory utilities,
address from, address to,
uint256[] memory amounts
) external globalUpdate {
require(msg.sender == address(nAstr) ||
msg.sender == adaptersDistributor, "Not access");
uint256 l = utilities.length;
for (uint256 i; i < l; i++) {
_transferDnt(utilities[i], from, to, amounts[i]);
}
}

Moreover, the _transferDnt() has a similar check which just returns if amount = 0 and doesn’t revert.

NFTDistributor::_transferDnt()

function _transferDnt(
string memory utility,
address from, address to,
uint256 amount
) private {
if (amount == 0) return;
...
}

It might would make sense to adopt one of the argument handling for these function which could be to either revert or not and just return when it is equal to 0.

A20

Compiler bugs

A20ADVISORY

Compiler bugs
info

The code can be compiled with Solidity 0.8.4 or higher. For deployment, we recommend no floating pragmas, but a specific version, to be confident about the baseline guarantees offered by the compiler. Version 0.8.4, in particular, has some known bugs, which we do not believe affect the correctness of the contracts.



DISCLAIMER

The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.


ABOUT DEDAUB

Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub's auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.

Dedaub