BadgerDAO security record

The current flow in the essential external functions of `mint` and `redeem` works by having most of the critical operations being performed and started in the peak contracts. The operations include the storing and collection/return of external tokens, retrieval of exchange rates, calculation of the actual exchange, and the emitting of event. The interaction with the `Core` contract comes in the form of the peak contracts telling it how many `bBTC` to `mint` or `redeem`, and the former following it without limits or any form of verification. Note also that the peak contracts may also `redeem` and therefore `burn` for any arbitrary account, even those that are not associated with it. This is a security model that relies on the peak contracts to be secure and correct. Given the definition of a peak in the `README.md` to be `any third party integration in the protocol`, we find that this security model is too trusting, and any malicious peak may either ruin the economics through excessive minting or loss of reputation from arbitrary `burn`ing.

In the `bBTC` contract, there is a method `switchCore` which is designed to be used to upgrade and switch `Core` contracts. This method is protected by the `onlyCore` modifier which allows only the existing `Core` contract to call it. However, given that the current `Core` implementation does not have any method that calls `switchCore`, it is likely that this method will never be callable, therefore rendering it useless.

Currently in `Zap`, the `mint` method does not check for whether the input `token` are the desired `wBTC` or `renBTC` addresses. Therefore, given that the only check for `token` is in the form of `safeTransferFrom` a specified amount, it is possible to set `token` to some arbitrary worthless token address to steal any underlying `wBTC` or `renBTC` on the `Zap` contract.

The `mint` function only validates for whether or not to `mint` to a particular `account` if `address(guestList) != address(0)`. The other access control present is the check for whether `peaks[msg.sender] == PeakState.Active`. This means that if `address(guestList) == address(0)`, an active peak can mint any arbitrary amounts of `bBTC`. Given that peaks are considered to be third party integrations, caution should be given to unbounded powers like this.

ibBTC resembles an index token across the different peaks that target different liquidity pools. Users currently can deposit into any of the pools to mint ibBTC and redeem it from any other arbitrary pools. This operates under the assumption that all BTC-derived tokens are equivalent in value, which may not always hold, and can allow for arbitrage of unbounded values. Building an index token does not necessarily require cross pool withdrawals as long as the value in shares and total supply calculations are done across pools.

It is currently possible for peaks with status of `PeakState.Dormant` to `redeem` in `Core`, and it is not clear, due to the lack of technical documentation around that, if this is intentional.

Magic numbers are constant numbers used in the middle, usually without description and requires some inference to understand what is going on. It would be far better as a security and software engineering practice to have it declared as a named state constant, much like it was done for `Core::PRECISION`. The following examples were discovered: 1. Core.sol::L114 | `1e18` 2. Core.sol::L116 | `1e18` 3. Zap.sol::L89 | `poolId + 2` 4. Zap.sol::L236 | `1e18` 5. BadgerSettPeak.sol::L118 | `1e18` 6. BadgerSettPeak.sol::L153 | `1e18` 7. BadgerSettPeak.sol::L172 | `1e36` 8. BadgerYearnWbtcPeak.sol::L102 | `1e18` 9. BadgerYearnWbtcPeak.sol::L127 | `1e20` 10. BadgerYearnWbtcPeak.sol::L145 | `100`

The `Core` contract receives and sends out amounts of `btc` in different decimal places to the peak contracts. We have identified the following problems that has caused us confusion in this audit: 1. Core.sol::L52 states that `BTC amount supplied, scaled by 1e18`, which should be `BTC amount supplied, scaled by 1e10` or `BTC amount supplied, in 18 decimal places` instead. 2. Core.sol::L92 states that `btc amount redeemed, scaled by 1e18`, which should be `BTC amount redeemed, scaled by 1e18` or `BTC amount redeemed, in 36 decimal places` instead, given that the `btc` returned is in 36 decimal places from `bBtcToBtc`. 3. Core.sol::L104 states that `btc amount redeemed, scaled by 1e36`, which should be `BTC amount expected, scaled by 1e18` or `BTC amount expected, in 36 decimal places`. It also does not appropriately name the `fee` that is returned. 4. Core.sol::L106 function name of `bBtcToBtc` should be renamed `bBtcToBtcScaled` to more accurately and pre-emptively warn any users of the function of the uniquely scaled effect. 5. BadgerSettPeak.sol::L143 states that `BTC amount, scaled by 1e36`, which should be `BTC amount, scaled by 1e18` or `BTC amount, in 36 decimal places`. 6. BadgerSettPeak.sol::L152 states that `is already scaled by 1e36` which should be `is already scaled by 1e18`. 7. BadgerYearnWbtcPeak.sol::L119 states that `BTC amount, scaled by 1e36` which should be `BTC amount, scaled by 1e18` or `BTC amount, in 36 decimal places`. 8. BadgerYearnWbtcPeak.sol::L126 states that `this value is scaled by 1e36` which should be `this value is scaled by 1e18` or `this value, in 36 decimal places`. 9. BadgerYearnWbtcPeak.sol::L134 states that `btc value, scaled by 1e18` which should be `BTC value, scaled by 1e10` or `BTC value, in 18 decimal places`. 10. BadgerYearnWbtcPeak.sol::L141-142 states that `wBTC and byvWBTC are scaled by 8 decimals. Multiply by 100 to return a value scaled by 1e18` which should be `wBTC and byvWBTC are in 8 decimals. Raise the result by 1e2 to return a value in 18 decimal places`.

Smart contracts will often have `owner` variables to designate the person with special privileges to make modifications to the smart contract. In this audit, we note the following cases: 1. In GuestList.sol, the `owner` is able to set the `guestRoot` either to `0` to completely remove any need for proof verification, or change it to anything else to invalidate all the previously working invitation proofs. 2. In GuestList.sol, the `owner` is able to set the `userDepositCap`and/or `totalDepositCap` to `0` or low enough such that any further user deposit will not be allowed. 3. In Core.sol, the `owner` is able to set any existing peak to the extinct state arbitrarily, therefore removing it from `totalSystemAssets` and heavily affecting the return for the `bBTC` token.

There are some setter functions for the contracts involved that does not compare the incoming parameter to the current state variable. This may result in a misleading event emitted or successful result being returned, and may confuse components monitoring for these changes. The following examples were spotted: 1. In Guestlist.sol, `setGuestRoot` does not validate for whether `guestRoot_` has changed. 2. In Guestlist.sol, `setUserDepositCap` does not validate for whether `cap_` has changed. 3. In Guestlist.sol, `setTotalDepositCap` does not validate for whether `cap_` has changed.