Safety Module - Biconomy

Safety Module contracts have address fields in multiple functions. These functions are missing address validations. Each address should be validated and checked to be non-zero. This is also considered as a best practice.

During testing, it has been found that some of these inputs are not protected against using address(0) as the target address.

Code Location

AaveDistributionManager.sol

constructor(address emissionManager, uint256 distributionDuration) public {
    DISTRIBUTION_END = block.timestamp.add(distributionDuration);
    EMISSION_MANAGER = emissionManager;
  }

BicoProtocolEcosystemReserve.sol

function _setFundsAdmin(address admin) internal {
    _fundsAdmin = admin;
    emit NewFundsAdmin(admin);
  }

StakedTokenBptRev2.sol

constructor(
    IERC20 stakedToken,
    IERC20 rewardToken,
    uint256 cooldownSeconds,
    uint256 unstakeWindow,
    address rewardsVault,
    address emissionManager,
    uint128 distributionDuration,
    string memory name,
    string memory symbol,
    uint8 decimals,
    address governance
  ) public ERC20(name, symbol) AaveDistributionManager(emissionManager, distributionDuration) {
    STAKED_TOKEN = stakedToken;
    REWARD_TOKEN = rewardToken;
    COOLDOWN_SECONDS = cooldownSeconds;
    UNSTAKE_WINDOW = unstakeWindow;
    REWARDS_VAULT = rewardsVault;
    _aaveGovernance = ITransferHook(governance);
    ERC20._setupDecimals(decimals);
  }

StakedTokenV3.sol

constructor(
    IERC20 stakedToken,
    IERC20 rewardToken,
    uint256 cooldownSeconds,
    uint256 unstakeWindow,
    address rewardsVault,
    address emissionManager,
    uint128 distributionDuration,
    string memory name,
    string memory symbol,
    uint8 decimals,
    address governance
  ) public ERC20(name, symbol) AaveDistributionManager(emissionManager, distributionDuration) {
    STAKED_TOKEN = stakedToken;
    REWARD_TOKEN = rewardToken;
    COOLDOWN_SECONDS = cooldownSeconds;
    UNSTAKE_WINDOW = unstakeWindow;
    REWARDS_VAULT = rewardsVault;
    _aaveGovernance = ITransferHook(governance);
    ERC20._setupDecimals(decimals);
  }

SOLVED: The issue was solved in commit ID: 20611133e6edd306f15840a2e5735577f1f08050

To protect against cross-function re-entrancy attacks, it may be necessary to use a mutex. By using this lock, an attacker can no longer exploit the withdrawal function with a recursive call. OpenZeppelin has its own mutex implementation called ReentrancyGuard which provides a modifier to any function called nonReentrant that protects the function with a mutex against re-entrancy attacks.

Code Location

Missing Reentrancy Guard - Functions

StakedTokenBptRev2::redeem()
StakedTokenBptRev2::claimRewards()
StakedTokenV3::redeem()
StakedTokenV3::claimRewards()

SOLVED: The issue was solved in commit ID: 20611133e6edd306f15840a2e5735577f1f08050

ABIEncoderV2 is enabled and using experimental features could be dangerous in live deployments. The experimental ABI encoder does not handle non-integer values shorter than 32 bytes properly. This applies to bytesNN types, bool, enum and other types when they are part of an array or a struct and encoded directly from storage. This means these storage references have to be used directly inside abi.encode(...) as arguments in external function calls or in event data without prior assignment to a local variable. The types bytesNN and bool will result in corrupted data, while enum might lead to an invalid revert.

Code Location

AaveDistributionManager.sol

pragma experimental ABIEncoderV2;

StakedTokenBptRev2.sol

pragma experimental ABIEncoderV2;

StakedTokenV3.sol

pragma experimental ABIEncoderV2;

ACKNOWLEDGED: The Biconomy team acknowledged this issue.

The project contains many instances of floating pragma. Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, either an outdated compiler version that might introduce bugs that affect the contract system negatively or a pragma version too recent which has not been extensively tested.

Code Location

Floating Pragma

AaveDistributionManager.sol::pragma solidity ^0.7.5;
DistributionTypes.sol::pragma solidity ^0.7.5;
StakedTokenBptRev2.sol::pragma solidity ^0.7.5;
StakedTokenV3.sol::pragma solidity ^0.7.5;
GovernancePowerDelegationERC20.sol::pragma solidity ^0.7.5;
GovernancePowerWithSnapshot.sol::pragma solidity ^0.7.5;
VersionedInitializable.sol::pragma solidity ^0.7.5;

SOLVED: The issue was solved in commit ID: 20611133e6edd306f15840a2e5735577f1f08050

In Solidiy, the exponentiation operation (**) costs up to 10 gas. It is possible to consume less gas to calculate the prices of the tokens if DECIMAL variable is fixed.

Code Location

AaveDistributionManager

function _getRewards(
    uint256 principalUserBalance,
    uint256 reserveIndex,
    uint256 userIndex
  ) internal pure returns (uint256) {
    return principalUserBalance.mul(reserveIndex.sub(userIndex)).div(10**uint256(PRECISION));
  }

AaveDistributionManager

uint256 currentTimestamp =
      block.timestamp > DISTRIBUTION_END ? DISTRIBUTION_END : block.timestamp;
    uint256 timeDelta = currentTimestamp.sub(lastUpdateTimestamp);
    return
      emissionPerSecond.mul(timeDelta).mul(10**uint256(PRECISION)).div(totalBalance).add(
        currentIndex
      );
  }

SOLVED: The issue was solved in commit ID: 20611133e6edd306f15840a2e5735577f1f08050

In public functions, the array arguments are immediately copied into memory, while external functions can read directly from the calldata. Reading calldata is cheaper than allocating memory.

Public functions need to write arguments to memory because public functions can be called internally. Internal calls are passed internally via pointers to memory. Therefore, a function expects its arguments to be located in memory when the compiler generates the code for an internal function.

Code Location

Misuse of Public Functions

AaveDistributionManager.getUserAssetData(address,address)
BicoProtocolEcosystemReserve.setFundsAdmin(address)
StakedTokenBptRev2.delegateByTypeBySig(address,IGovernancePowerDelegationToken.DelegationType,uint256,uint256,uint8,bytes32,bytes32)
StakedTokenBptRev2.delegateBySig(address,uint256,uint256,uint8,bytes32,bytes32)

SOLVED: The issue was solved in commit ID: 20611133e6edd306f15840a2e5735577f1f08050

In all the loops, the variable i is incremented using i++. It is known that, in loops, using ++i costs less gas per iteration than i++. This also affects variables incremented inside the loop code block.

Code Location

AaveDistributionManager.sol

for (uint256 i = 0; i < assetsConfigInput.length; i++) {
      AssetData storage assetConfig = assets[assetsConfigInput[i].underlyingAsset];

AaveDistributionManager.sol

for (uint256 i = 0; i < stakes.length; i++) {
      accruedRewards = accruedRewards.add(
        _updateUserAssetInternal(

AaveDistributionManager.sol

 for (uint256 i = 0; i < stakes.length; i++) {
      AssetData storage assetConfig = assets[stakes[i].underlyingAsset];

ACKNOWLEDGED: The Biconomy team acknowledged this issue.