Read the findings of the 2022 Web3 Security Report

The contract is serving as the underlying entity to interact with third party `registry` protocols. The scope of the audit treats 3rd party entities as black boxes and assumes their functional correctness. However, in the real world, 3rd parties can be compromised and this may lead to lost or stolen assets. In addition, upgrades of 3rd parties can possibly create severe impacts, such as increasing fees of 3rd parties, migrating to new LP pools, etc.

Third Party Dependencies

```solidity=82
    function _updateFee(uint256 _fee) internal {
        require(_fee >= 0 && _fee <= 10000, "IF");
        uint256 oldFee = fee;
        fee = _fee;
        emit FeeSet(msg.sender,oldFee, fee);
    }
```
The fee rate could be updated to a maximum value of 100%, which means depositors would not get any reward.

No Reasonable Upper Limit for `fee`

Variable `thresholdMinimumFeeCollection` could be used as the threshold for `feeCollector` to collect fees. However, it is only used to emit an event in contract `UnbkV1`.

Variable `thresholdMinimumFeeCollection` May Not Be Handled Properly

`BaseRouterUpgradeableV1` implements the interface `IBaseRouterUpgradeable`, but does not inherit from it.

`FeeUpgradeableV1` implements the interface `IFeeupgradeable`, but does not inherit from it.

Missing Inheritance

In the contract `BaseRouterUpgradeableV1` the role `registry.governance()` has authority over the function `setRegistry()` that can update the address of `registry`.

In the contract `UnbkRolesUpgradeableV1`:
- the role `DEFAULT_ADMIN_ROLE` has authority over the functions `setupContractUpdater()` and `setupTreasuryRole()`
- the role `CONTRACT_UPDATER` has authority over the function `setupBonusRewarderRole()`

In the contract `FeeUpgradeableV1`:
- the role `owner` has authority over the function `setRoleContract()` 
- the role `CONTRACT_UPDATER` has authority over the functions `updateFee()` and `updateThresholdMinimumFeeCollection()`
- the role `TREASURY` has authority over the function `updateFeeCollector()`

In the contract `UnbkV1`:
- the role `BONUS_REWARDER` has authority over the function `withdraw(uint256 shares, uint256 deadline, uint256 _fee, uint8 v, bytes32 r, bytes32 s)`
- the role `TREASURY` has authority over the functions `claimFees()` and `claimFees(uint256 amount)`
- the role `CONTRACT_UPDATER` has authority over the functions `migrate()`, `migrate(uint256 amount)`, and `migrate(uint256 amount, uint256 maxMigrationLoss)`
- the role `_owner` has authority over the functions shown in the diagram below.

Any compromise to these account may allow a hacker to take advantage of this authority and damage the tokenomics.

![](https://accelerator-tasks-prod.s3.amazonaws.com/4ffab37c28f84545ba826b087d03d773/diagrams/centralization-UnbkV1-_owner.svg)

Centralization Related Risks

Contracts `FeeUpgradeableV1`, `BaseRouterUpgradeableV1`, and `UnbkRolesUpgradeableV1` are upgradeable contracts, the owner can upgrade the contract without the community's commitment. If an attacker compromises the account, he/she can change the implementation of the contract and manipulate critical functions.

Centralized Control of Upgradeable Contracts

The **require** can be used to check for conditions and throw an exception if the condition is not met. It is better to provide a string message containing details about the error that will be passed back to the caller.

Missing Error Messages

```solidity=479
    function _withdraw(
        address sender,
        address receiver,
        uint256 amount, // if `MAX_UINT256`, just withdraw everything
        bool withdrawFromBest, // If true, also withdraw from `_bestVault`
        uint256 sharesToInvestedTokens,
        uint256 _fee,
        uint256 _thresholdMinimumFeeCollection
    ) internal returns (uint256 withdrawn) {
        withdrawn =  _withdrawFromVaults(token,sender,amount,withdrawFromBest);
        uint256 deductedTokenFees = feeResolver.determineFee(
            withdrawn,
            sharesToInvestedTokens,
            receiver,
            _fee
        );
        // `receiver` now has `withdrawn` tokens as balance
        if (receiver != address(this)) {
            withdrawn = withdrawn - deductedTokenFees;
            SafeERC20Upgradeable.safeTransfer(token, receiver, withdrawn);
        }
        bool isCollectorTriggerer = token.balanceOf(address(this)) >=
            _thresholdMinimumFeeCollection;
        emit FundsWithdrawn(
            receiver,
            deductedTokenFees,
            sharesToInvestedTokens,
            withdrawn,
            isCollectorTriggerer
        );
    }
```
If a user calls function `withdraw()` and it would trigger the function `_withdraw()` above. If variable `deductedTokenFees` is not 0, then the number of tokens would be left in this contract as fees.

Then if someone calls function `deposit()`, the fees leftover would be deposited as a part of his/her tokens. 

Since the fees can only be claimed by the role `TREASURY` manually through function `claimFees()` or `claimFees(uint256 amount)`, the situation above is very likely to happen.

Potential Fee Loss

If the token being deposited is a deflationary token implementing the ERC20 standard, the input `amount` may not be equal to the received amount due to the charged transaction fee. In this case, if a user deposits `amount` deflationary tokens (with a non-zero transaction fee), fewer than `amount` tokens actually arrive in the contract. 

If `amount != DEPOSIT_EVERYTHING`, `_bestVault.deposit(amount)` at line 615 would revert because the balance of this contract is less than `amount`.

Potential Incompatibility With Deflationary Tokens

Function `_deposit()` calls `SafeERC20Upgradeable.safeApprove(_token, address(_bestVault), 0)` at line 599 to avoid issues with some tokens requiring approving 0 before approving `UNLIMITED_APPROVAL`.

However, function `_withdrawFromVaults()` does not approve 0 before approve `UNLIMITED_APPROVAL`.

Missing `safeApprove(_token, address(_bestVault), 0)`

```solidity=275
    function migrate() external returns (uint256) {
        _verifyContractUpdaterRole();
        return _migrate(address(this));
    }

    function migrate(uint256 amount) external returns (uint256) {
        _verifyContractUpdaterRole();
        return _migrate(address(this), amount);
    }

    function migrate(uint256 amount, uint256 maxMigrationLoss)
        external
        returns (uint256)
    {
        _verifyContractUpdaterRole();
        return _migrate(address(this), amount, maxMigrationLoss);
    }
```
The functions above do not apply modifier `whenNotPaused`.

No Modifier `whenNotPaused`

```solidity=293
    function _migrate(address account) internal returns (uint256) {
        return _migrate(account, MIGRATE_EVERYTHING);
    }
```
In function `_migrate(address account, uint256 amount, uint256 maxMigrationLoss)`
```solidity=319
   _depositLimit < UNCAPPED_DEPOSITS && _amount < WITHDRAW_EVERYTHING
```
Variable `WITHDRAW_EVERYTHING` should be replaced with `MIGRATE_EVERYTHING` to eliminate inconsistency.

This issue does not affect the functionality because their values are the same.

`MIGRATE_EVERYTHING` Rather Than `WITHDRAW_EVERYTHING`

Unbanked

BaseRouterUpgradeableV1.sol

UnbkV1.sol

VaultAPI.sol

Unbanked

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

12

0

4

0

8