Read the findings of the 2022 Web3 Security Report

In the smart contracts `whalefogERC20` and `whalefogBNB`, there is `bnbFee` defined and used to claim Ether(BNB) in Deposit operation.\
The `relayfee` is already defined to be sent to the relayer as reward in both contracts, in ERC20 the relay fee is ERC20 token and Ether(BNB) in `whalefogBNB`. What's the purpose to add `bnbFee` as Ether(BNB) in both contracts?\
Besides, this is no way to withdraw these Ether(BNB) which will be locked in both conttracts.

`bnbFee` Locked in  `whalefogERC20` and `whalefogBNB` contracts

The `MerkleTreeWithHistory` contract forked from Tornado is serving as the underlying entity to interact with MiMC Hash.
Both `whalefogERC20` and `whalefogBNB` contracts interact with verifiers of Deposit and Withdraw of which the implementation is non-transparent and could be changed by the owner of both contracts. 

The scope of the audit treats 3rd party entities as black boxes and assume the 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 protocols, etc.

Third Party Dependencies

The contract has unlocked compiler version. An unlocked compiler version in the source code of the contract permits the user to compile it at or above a particular version. This, in turn, leads to differences in the generated bytecode between compilations due to differing compiler version numbers. This can lead to an ambiguity when debugging as compiler specific bugs may occur in the codebase that would be hard to identify over a span of multiple compiler versions rather than a specific one.

Unlocked Compiler Version

The project adopt Tornado Cash Privacy Solution which implements zero-knowledge privacy in which the smart contracts accept transactions in BNB or ERC20 tokens and later the amount can be withdrawn with no reference to the original transaction.\
The contracts for both BNB and ERC20 include two main functions:
* Deposit property to the contracts.
  * It accepts `_balance` amount with commitment data `_commitment` and zero-knowledge proof, but also Ether(BNB).
  * It verifies that commitment is not used in the Merkel tree, `_balance` and the sent Ether meet the floor boundary values.
  * It verifies the zero-knowledge proof.
  * Once verified successfully the amount will be transferred to the contract along with Ether(BNB) and the commitment will be added to the Merkel tree, the path from the last added value and the latest root is recalculated.
* Withdraw property from the contract. The amount is withdrawn through a relayer with relay fee sent to Relayer address and the remaining to the recipient address. The value of relay fee and address is set by contract owner. Apart from the fixed relay fee, the caller could give additional fee which should be less than relay fee to the relayer.
  * It accepts `_amount` money with commitment, zero-knowledge proof, root of Merkel tree, proof usage of commitment data `_voucher`, recipient address, relayer address, fee and refund.
  * It verifies that commitment is not used in the Merkel tree, voucher is not used, additional fee and amount meets the boundary values.
  * It verifies the zero-knowledge proof and the provided root of Merkel tree.
  * Once verified successfully it will transfer the calculated amounts to recipient and relayer and add the commitment to the Merkel tree.

Financial Models

In the contract `whalefogBNB` the role `operator` has authority over the functions shown as below:
- `setFee()`: set relayer fee, bnb fee and minimum deposit/withdraw balance.
- `updateDPVerifier()`: change deposit verifier which will affect deposit.
- `updateWDVerifier()`: change withdraw verifier which will affect withdraw.
- `changeOperator()`: change operator.

In the contract `whalefogERC20` the role `operator` has authority over the functions shown as below:
- `setFee()`: set relayer fee, bnb fee and minimum deposit/withdraw balance.
- `updateDPVerifier()`: change deposit verifier which will affect deposit.
- `updateWDVerifier()`: change withdraw verifier which will affect withdraw.
- `updateToken()`: change token which will affect depost and withdraw.
- `changeOperator()`: change operator.

Any compromise to the privileged account may allow the hacker to take advantage of this authority.

Centralization Risks

SafeMath from OpenZeppelin is not used. The following functions may suffer from overflow/underflow and lead to an inaccurate calculation result.

File: whalefog-erc20.sol (Line 147, Function `whalefogERC20._processWithdraw`)
```solidity=147
    token.transfer(_recipient, reamount - _fee);
```

---
File: whalefog-bnb.sol (Line 60, Function `whalefogBNB.deposit`)
```solidity=60
    require(msg.value  >= (bnbfee + _balance),"msg value is less than bnb fee ");
```

---
File: whalefog-bnb.sol (Line 126, Function `whalefogBNB._processWithdraw`)
```solidity=126
    _recipient.transfer(reamount - _fee);
```


SafeMath Not Used

In the `setFee()` function, there is no reasonable ceiling limit check for `min` or `bnbfee` which may be assigned to arbitrary values.

File: whalefog-erc20.sol (Line 116, Function `whalefogERC20.setFee`)
```solidity=116
  function setFee(uint256 _bnbfee, uint256 _relayfee, uint256 _min) external onlyOperator {
    require(_relayfee < 100000, "repalyfee should less than 100000");
    bnbfee = _bnbfee;
    relayfee = _relayfee;
    min = _min;
  }
```

---
File: whalefog-bnb.sol (Line 100, Function `whalefogBNB.setFee`)
```solidity=100
  function setFee(uint256 _bnbfee, uint256 _relayfee, uint256 _min) external onlyOperator {
    require(_relayfee < 100000, "repalyfee should less than 100000");
    bnbfee = _bnbfee;
    relayfee = _relayfee;
    min = _min;
  }
```

No upper boundary check for both `bnbfee` and `min`.

Lack of Ceiling Limit in `setFee` function

Addresses should be checked before assignment or external call to make sure they are not zero addresses.

File: whalefog-bnb.sol (Line 117, Function `whalefogBNB.changeOperator`)
```solidity
    operator = _newOperator;
```
- `_newOperator` is not zero-checked before being used.


---
File: whalefog-erc20.sol (Line 136, Function `whalefogERC20.changeOperator`)
```solidity
    operator = _newOperator;
```
- `_newOperator` is not zero-checked before being used.



Missing Zero Address Validation

It's not proper to emit a `Deposit` event using the withdrawn amount in withdraw operation where the new commitment represents the remaining amount after current withdraw.
```solidity
uint32 insertedIndex = _insert(_commitment);
emit Deposit(_commitment, uint256(_amount), insertedIndex, block.timestamp);
```
In the code snippet, `_commitment` is not matched with `_amount` in such event and also makes confusion.

Improper Emitting `Deposit` Event

There should always be events emitted in the sensitive functions that are controlled by centralization roles.

Missing Emit Events

The linked functions are equivalent to the compiler-generated getter functions for the respective variables.

User-Defined Getters

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

The implementation of contract `ReentrancyGuard` mentioned in both *.sol files comes from an early version([v2](https://github.com/OpenZeppelin/openzeppelin-contracts/blob/release-v2.3.0/contracts/utils/ReentrancyGuard.sol)) of Openzeppelin.\
According to [EIP 2200](https://github.com/ethereum/EIPs/blob/e4d4ea348e06c54d0075c400dc7b72430d427ff1/EIPS/eip-2200.md), which implements "net gas metering" for `sstore` operations. If the final value of _guardCounter is unchanged relative to the original value of it, a gas refund will be applied and charges for changing the value of _guardCounter will effectively not exist.\
This ends up being cheaper than the current implementation after Istanbul Hard Fork which has been completed.

Inefficient ReentrancyGuard Implementation

The given input is missing the check for the non-zero address.

Missing Input Validation

The linked variables are assigned only once, either during their contract-level declaration or during the `constructor`'s execution.

Mutability Specifiers Missing

The function call that sends Ether to an arbitrary address is unprotected.

File: whalefog-bnb.sol (Line 126, Function `whalefogBNB._processWithdraw`)
```solidity
      _recipient.transfer(reamount - _fee);
```

File: whalefog-bnb.sol (Line 130, Function `whalefogBNB._processWithdraw`)
```solidity
        _relayer.transfer(relayamount);
```

File: whalefog-bnb.sol (Line 135, Function `whalefogBNB._processWithdraw`)
```solidity
        _relayer.transfer(_fee);
```



Sending Ether to Arbitrary Destinations

After [EIP-1884](https://eips.ethereum.org/EIPS/eip-1884) was included in the Istanbul hard fork, it is not recommended to use `.transfer()` or `.send()` for transferring ether as these functions have a hard-coded value for gas costs making them obsolete as they are forwarding a fixed amount of gas, specifically `2300`. This can cause issues in case the linked statements are meant to be able to transfer funds to other contracts instead of EOAs.

Usage of `transfer()` for sending Ether

1. The boolean expression at L72 returns false and displays the error message if the value of `_fee` is greater than the relay fee. So the error message should be `Fee exceeds relay fee`.
2. The boolean expression at L60 returns false and displays the error message if the `msg.value` is less than bnb fee and deposit amount. So the error message should be `msg value is less than bnb fee and deposit amount`.

Inconsistent Comment and Code

The return value of the transfer()/transferFrom() call is not checked.

File: whalefog-erc20.sol (Line 147, Function `whalefogERC20._processWithdraw`)
```solidity
      token.transfer(_recipient, reamount - _fee);
```


---
File: whalefog-erc20.sol (Line 151, Function `whalefogERC20._processWithdraw`)
```solidity
        token.transfer(_relayer, relayamount);
```


---
File: whalefog-erc20.sol (Line 156, Function `whalefogERC20._processWithdraw`)
```solidity
        token.transfer(_relayer, _fee);
```



Unchecked ERC-20 `transfer()`/`transferFrom()` Call

In smart contract `whalefogERC20` and `whalefogBNB`, sender may over pay Ether(BNB) in deposit function and there is no refund logic.
```solidity
require(msg.value >= bnbfee,"msg value is less than bnb fee ");
```
```solidity
require(msg.value  >= (bnbfee + _balance),"msg value is less than bnb fee ");
```

WhaleFog

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

20

0

8

1

11