Fabric CrowdFinancing 2 security record

When contributions are transferred between accounts, the `_withdraws` mapping must be updated to accurately reflect the yield balance when the `withdrawYieldBalance()` function is called. However, the current implementation is flawed, resulting in incorrect values for `_withdraws[from]` and `_withdraws[to]`. Specifically, the formula used to calculate `withdrawAmount` is incorrect, and should be updated from: ``` withdrawAmount = (fromBalance - amount) * fromWithdraws) / fromBalance; ``` To: ``` withdrawAmount = (amount * fromWithdraws) / fromBalance; ``` This error can be exploited by a malicious user to withdraw all of the available yield balance from the contract by executing the following steps multiple times until the contract balance is empty: 1. The malicious user transfers a contribution of zero value to a random address, setting its `_withdraws[from]` value to zero. 2. The malicious user executes `withdrawYieldBalance()` to withdraw its generated yield again.

The way the yield fee is calculated is by increasing the total contribution by adding a percentage of the total contributions and awarding it to the fee recipient as follows: ```solidity uint256 feeAllocation = (_contributionTotal * _feeYieldBips) / (MAX_BIPS); _contributions[_feeRecipient] += feeAllocation; _contributionTotal += feeAllocation; ``` However, this formula means that the fee recipient is taking a value less than `_feeYield / MAX_BIPS`.

Since contributions are handled using the `ERC20` standard, `withdrawContribution()` should also update `_contributionTotal` accordingly.

There are several checks in the `contributionGuard()` function to ensure contributions are allowed according to `_minContribution` and `_maxContribution`. The `_safeTransfer()` function does not necessarily return the input amount value but the real deposited value calculated between the previous and current balance of the contract. Because of that, in the case of a fee on transfer token, these checks may fail to control the real amounts being contributed.

`minContribution` is checked twice against the minimum and maximum funding goals. ``` require(minContribution <= maxGoal, "Min contribution must be <= Target Max"); require(minContribution < (maxGoal - minGoal), "Min contribution must be < (maxGoal - minGoal)"); ``` Note that the following `require()` statement suggests that it should be possible for `minGoal` to be equal to the `maxGoal` ``` require(minGoal <= maxGoal, "Min target must be <= Max"); ``` If this is intentional, then the require statement checking that `minContribution < (maxGoal - minGoal)` is problematic, since it can never be true in the case where `maxGoal == minGoal`.

Not all ERC20 implementations respect the ERC20 standard, multiple adopted tokens do not return any value when executing `ERC20.transfer()` or even `ERC20.transferFrom()`, the implemented requirements in the following functions might not work as expected: - `transferBalanceToRecipient()` calls `ERC20.transfer()` multiple times. - `_transferSafe()` calls `ERC20.transferFrom()` - `withdrawContribution()` calls `ERC20.transfer()`. - `withdrawYieldBalance()` calls `ERC20.transfer()`.