# Floor DAO Findings and Review Log

The review was three weeks of the v2 of the Floor DAO codebase. The goal of this code is to automate more of the onchain actions the DAO currently preforms via multisig and to increase transparency regarding votes and yield from the NFT LP positions. Notably the contracts still contain strong governance control points which would allow some addresses to remove the DAO assets.

During the course of the review I have discovered 7 critical bugs, 5 high bugs, 5 medium bugs and 6 low bugs, plus various notes on gas and edge cases. These rating are judged against a matrix defined by how likely a bug is and how much damage the bug could cause. The status of the reviewed remediation is indicated along side each entry in my log.

The commit hash at the start of review was: 74bec48f0dcdd08cdb2fcee89f24284a58124442
The ending commit:

Please note that no security review can be exhaustive, and I'm a human being who can and does miss bugs. This review only serves as a way for the team to spot check the codebase and to develop security methodology.

## Log of Issues noted, sorted chronologically

* Delete Trigger Bricks Epoch Manager: the epoc manager's deletion function sets addresses to zero, but doesn't resize the trigger array. When calling endEpoc a call to zero is forced and will revert. Future updates cannot remove the zero value as they use array.push() which cannot overwrite elements.
* Severity Estimate: High [Likelyhood: Medium, Impact: High]
* Remediation Recommendation: Resize the array instead of using 'delete[index]', add testing of the delete flow.
* Status: Resolved
* Gas Exhaustion in processAction: The treasury's processAction function takes an array of actions and does approvals corresponding to each. In it a for loop is missing an assignment of the loop index and so will run forever.
* Severity Estimate: High [Likelyhood: High, Impact: Medium]
* Remediation Recommendation: Add index assignment and tests of this function
* Status Resolved
* Reentrancy in sweepEpoch: The sweepEpoc function checks that the epoc has not been swept by checking a storage variable 'completed' has not been set to true. However it then calls the sweeper with arbitrary data, and some sweepers may allow external calls. This lets a malicious user with 5000 floor potentially execute sweeps many times by having the sweeper call back into the sweep function, this may buy many more NFTs than intended and drain the weth in treasury.
* Severity Estimate: High [Likelyhood: Low - Medium, Impact: High]
* Remediation Recommendation: Store the completed flag at the beginning of the internal sweep function.
* Status: Resolved
* Infinite Vote for collection in NewCollectionWar: If a user has already voted the vote function only adds the delta of their new votes since the last vote to the new collection. The revoke function removes all votes from the current collection in an unchecked block. In this case the subtract will underflow and the collection votes will be set to on the order of 2\*\*255 which garuntees the collection will win.
* Severity Estimate: Critical [Likelyhood: High, Impact: High]
* Remediation Recommendation: Move the whole voting power not just the delta to the new collection when the user changes votes.
* Status: Resolved
* Persistent Access for Treasury Manager Role: If a user has the process action role they can maintain access to ERC20 and ER721 even if the role is removed. They can do so by calling process action and approving a metamorphic contract which is self destructed at the end of the call. Since the contract does not exist calls to IAction(target).execute() will fail, so the approvals cannot be decremented by other admins. However they can still access assets by redeploying the contract and calling 'transferFrom'.
* Severity Estimate: Note/Low [Malicous managers could just steal assets so this is very unlikely]
* Remediation Recommendation: Add a decrement approval function.
* Status: Resolved
* GemSweeper Funds Theft: The GemSweeper contract can be used by a malicious sweeper to steal sweep proceeds. The GemSweeper decodes the address for gem from the user provided calldata and then calls it with the sweep value. This function can be called by the treasury with the sweepEpoc function, provided a user has 5k floor and it was not executed by the team earlier. Since the GemSweeper contract is approved the user can call it with the sweep value and then call their own contract and keep the funds.
* Severity Estimate: Critical [Likelyhood: Medium - High, Impact: High]
* Remediation Recommendation: Preset known Gem contracts to ensure the user provides an on malicious one
* Status: Resolved
* Note: This is a vector for the previously mentioned reentrancy and enables draining the treasury of all weth. Given this bug I would recommend prioritizing fixing the reentrancy, even if all known vectors are closed.
* EarlyWithdraw reverts after 24 epocs: The early withdraw function calculates 'epositor.epochCount - depositor.epochStart' where start epoch is monotonically increasing (weekly in planned deployment) but epochCount < 24. Since this is in a checked block any underflow here will revert and so each early withdraw after 24 weeks reverts.
* Severity Estimate: High [Likelyhood: High, Impact:
Medium]
* Remediation Recommendation: Refactor to avoid reverts.
* Status: Resolved

* Early withdraw penalty is calculated incorrectly: The early withdraw penalty is intended to be calculated as a percent of the user's lock remaining, so for example in a 2 week lock on week 1 the withdraw percent is 50%. In the calculation however the code assumes the user has the maximum lock length so in this scenario the withdraw percent as written is 5%. Users are unlikely to take real loss as the code only allows a certain max percent loss, and the function will just revert.
* Severity Estimate: Medium [Impact: Low, Likelihood: High]
* Remediation Recommendation: Divide by the user's lockTime not the max time.
* Status: Resolved

* Prices are calculated wrong in \_calculateFloorPrice: The function takes in two price ratios (floor to eth, token to eth) and then reverse engineers the price for floor by dividing. But it does the check priceFloor > priceToken and flips the division based on the outcome. This will generate incorrect results when floorPrice < tokenPrice. There is low impact because this function is not used anywhere else in the codebase.
* Severity Estimate: Note [Impact: Note, Likelihood: High]
* Remediation Recommendation: Fix the math and test
* Status: Open
* Prices are not updated when a collection is not approved: The register sweep trigger calls the price calculation function on an array of approved collections from the collection manager and then stores the result in a mapping. If a collection is approved and then un-approved the prices stop updating but the mapping still holds price data so any strategies which yield that token will return incorrect yield amounts. This also affects tokens which are never approved which might be created by the revenue strategies. Namely if a strategy is approved which is not producing yield in terms of the the approved collections is always credited as 0 eth.This has an impact in practice as when the liqudiation trigger liqudiates a collection it produces wETH and puts the wETH rewards into a distributed revenue strategy based on wETH. If the rewards are queried for this strategy since the wETH field of the tokenEthPrice is never set the rewards will always read as zero.
* Severity Estimate: Medium [Impact: Medium, Likelihood: Medium]
* Remediation Recommendation: Ensure all rewards tokens are queried for current prices via the price executor.
* Status: Open
* Missing Decimal Factor in RegisterSweep: The price executor returns prices as an 18 point decimal representation with the units of (eth/token). When we multiply by price in this [line](https://https://github.com/FloorDAO/floor-v2/blob/74bec48f0dcdd08cdb2fcee89f24284a58124442/src/contracts/triggers/RegisterSweep.sol#L118) we should also be dividing by the decmials of our token, otherwise the rewards will be inflated by 10^{token decimals}. This causes very large incorrect reports of sweep values to the treasury and would brick the system in production.
* Severity Estimate: Critical [Impact: High, Likelihood: High]
* Remediation Recommendation: Divide by token decimals
* Status: Resolved
* Missing User State Update in NewCollectionWars Vote: Currently the user's vote in new collection wars is checked by checking the mapping userCollectionVote[warUser] however when the user votes this entry is never set and so the user cannot change votes or the manager revoke their votes.
* Severity Estimate: Medium [Impact: Low; Likelihood: High]
* Remediation Recommendation: Add the users voted collection to the war's mapping.
* Status: Resolved
* Wrong collection's votes decremented in NewFloorWar: In the NewCollectionWar's voting function first it hashes the user provided collection with the war index to get a constant `warCollection` this constant is entirely derived from the user inputs. The contract decrements via `collectionVotes[warCollection] -= userVotes[warUser];` and so the system does not reduce votes from the user's currently voted collection but rather the one for which they are now trying to vote. This means the user's vote totals are unchanged as those votes are then added again to `collectionVotes[warCollection]` a few lines later. In theory this should change what contract the `userCollectionVote`is [dependant on the remediation of another bug]. If however the vote manager roles were to revoke votes from that account it would decrement the wrong collection and possibly cause an underflow. So given the following steps a user could create infinite votes: vote for collection A, then vote for collection B, then withdraw from veFloor paying the early withdraw fee which triggers a revoke of their vote in new collection war contract.
* Severity estimate - Critical [Impact: High, Likelihood: High]
* Remediation recommendation: Decrement from the hash of the war index and 'userCollectionVote[msg.sender]' instead of the user provided value.
* Status: Resolved
* Uniswap Sandwich on Liquidate: The liquidate function requires the trade to return zero results from swapping tokens to wETH. Uniswap v3 in particular has cases where this is very dangerous as the liquidity could be concentrated in a range and a sandwich could remove sale liquidity and add LP for this trade at a very low price netting the pool very close to zero. They could then trade the output back into the pool and steal the whole value of the liquidated amount. The code in the liquidation trigger is triggered by an non-permissioned call from the epoch manager meaning it is quite easy to execute this.
* Severity Estimate - Critical [Impact: High, Likihood: High]
* Remediation Recomendation - Use the prices from the pricing executor to ensure a bounded slippage.
* Status: Resolved
* Uniswap TWAP can return manipulated data: In the uniswap pricing executor the code attempts to load the data with a 30 minute lag, and if it can it will then try to calculate the most recent data it can. In some configurations of the pools this is as recent as the same block or 1 block ago. Such data is easy to manipulate in the current MEV environment, and may be fairly low cost given that the amount volume needed to perturb a pool is not constant across price regimes in v3. I would also consider using a longer TWAP for assets which may show low volume or in active trading. Manipulation could result in larger sweeps for some collections.
* Severity Estimate - Low [Impact: Medium, Likelihood: Low]
* Remediation Recommendation - Enforce a min oracle time average.
* Status: Open
* Manual Sweeper makes ETH unretrievable: The manual sweeper is called by the treasury contract with an amount of eth which is set by the sweep execution trigger. The manual sweeper function contains no way to retrieve this eth and so if it is called the ETH is burned.
* Severity Estimate - High [Impact: High, Likelihood: Medium]
* Remediation Recommendation - Add a path for an owner to retrieve eth
* Status: In Process
* Vote Revoking Edge Cases allow double voting: In the SweepWars contract the votes are allowed for either collections in the approved collections mapping or for the constant address 0x1. The revoke function however only revokes votes for the the collections in the approvedCollections array, which does not contain 0x1. Therefore a user can vote for 0x1 and then withdraw from vFloor and retain this vote. In another case if a collection is unapproved all of the votes are frozen and all users can withdraw and then redeposit, and vote for that collection again. If this was executed as coordinated attack a collection could be made permanently the higest voted collection.
* Severity Estimate - Low [Impact: Low, Likelihood: High]
* The votes for 0x1 are not used anywhere as only the approved collections are snapshotted for sweeps, so while infinite votes can be made they are not used.
* Remediation Recommendation: Ensure all user votes are cleared.
* Status: Resolved
* Locking Vulnerability in UniswapStrategy: The uniswap strategy calls the position manager element of uniswap's periphery contracts. This contract returns the current amount of liquidity and the amount of each token consumed by this deposit. The contract sets all three values into a positions mapping, and the positions mapping is decremented when a user withdraws. If two users deposit and the second one deposits only very small amount all withdraws are blocked of the first user's assets, because they would subtract a larger number from a smaller number in a checked arithmetic block. All previous deposits become unrecoverable.
* Severity Estimate - Critical [Impact: High, Likelihood: High]
* Remediation Recommendation: Remove the positions mapping entirely. The values are not used and lesser impact version of this bug could take place as the pool shifted its token composition over time.
* Status: Resolved
* Re-entrancy In UniswapStrategy: The wETH refund mechanic can trigger execution in the caller of deposit again. This user could then callback in and cause the following problem: deposit stores the total liquidity tokens of the pool after the refund mechanic. Since that value is calculated before and stored after the re-entrant call you can use the reentrancy to store totalLiqudityTokens < realBalance. There's no reason to do this because the value isn't stored on chain but if future changes are made to this contract care should be made to avoid relying on that property.
* Severity Estimate: Note [Impact: Low, Likelihood: Low]
* Remediation Recommendation: Checks effects interactions pattern.
* Status: Resolved
* UniswapStrategy Deposit Always Reverts for some Expected Inputs: The UniwswapStrategy deposit function uses the TokenUtils library which accepts uint.max as a value to mean "use the current token balance". The function deposits to uniswap via the position manager which returns the amounts used. The function the calls withdraw with "amountDesired - amountUsed" which will be almost 2^256, and the safe transfer will always revert. This prevents access to an intended code path but is not a major affect as that path is not the primary way to use this function.
* Severity Estimate: Low [Impact: Medium/Low, Likelihood: Low]
* Remediation Recommendation: Remove the use of the token utils library to make this contract uniform with the rest of the codebase.
* Status: Open.
* Vote Duplication in Sweep Wars: The sweep wars contract records a user's voting power based on the ratio at the beginning of their vote. When votes are removed the contract calculates the user's current voting power based on their current locking ratio and then either removes those votes if the vote is for for a collection or adds them if the vote is against a collection. The consequence of this is if a user can either increase or reduce their voting power after voting some remainder will be left when they withdraw at the end because the voting power ratio at the end of the locking period is not equal to what it was at the beginning. This allows vote duplication via the following steps:
1) Deposit at the lowest possible lock length (2 weeks)
2) Vote in the opposite of the preferred direction (with 1/12 ie about 8.6% of total votes)
3) Extend lock by depositing again to the longest lock length (24 weeks)
4) Wait for lock to end and withdraw causing the correction to overshoot because the user's current voting power is 100% of what was deposited (by aprox 91.4% with current constants)
5) Deposit and vote again and you will have added 191% of your voting power to your preferred collection.
* Severity Estimate: Critical [Impact: High, Likelihood: High]
* Remediation Recommendation: Store real user votes not their total amount of floor voted.
* Status: Open
* Optimal Voting for Large Voting power collections: If a collection will have greater than 50% of the net votes in favor of it then voters for it can engage in a manipulation attack. This is because the snapshot method and the liquidation method both use the net voting power from the voting contract, meaning you can reduce the numerator. In the extremal case if 75% of a vote is for a collection and 25% against it can optimize votes to get 100% of rewards [by voting 50% in favor and 25% against all other collections].
* Severity Estimate: Low [Impact: Medium, Likelihood: Low]
* Remediation Recommendation: Store and read the gross vote totals when computing percents.
* Status: No Action
* Vote Miscount in NewFloorWars: The new floor wars contract only calculates the user's balance but not their voting power. This means that both users who have locked for the max duration and min duration get the same votes if they deposited the same number of floor tokens.
* Rating: Medium [Impact: Low, Likelihood: High]
* Remediation Recommendation: Query the user votes when they vote via the veFloor.votingPowerOfAt function.
* Status: Open
* Rewards Miscount in DistributedRevenueStaking Strategy: The available() function returns the currently withdrawable total plus lifetime rewards. When it is called by the total rewards function the lifetime rewards are added again. Therefore when snapshot calls total rewards the function loads the currently claimable rewards plus two times lifetime rewards. This causes the strategy to payout twice the intended rewards as it will pay them out once when they are pending and then again when they are doubled in lifetime rewards.
* Rating: Medium [Impact: Medium, Likelihood: Medium]
* Remediation Recommendation: Don't add lifetime rewards to the available function.
* Full Withdraws from NFTX strategies Result in Deposit Locking: The deposits state mapping is decreased in an unchecked block on all withdraws from the NFTXInventoryStakingStrategy and NFTXLiqudityPoolStakingStrategy however it is increased only in the ERC20 deposit methods (in a checked block) and so if NFTs are deposited and then withdrawn the deposits mapping can underflow and then if future ERC20 deposits will revert as the overflow would revert in the checked block. This is considered unlikely to have an impact as it must be fully withdrawn in the course of normal use while having NFT deposits to lock only one method.
* Rating: Low/Note [Impact: Medium, Likelihood: Low]
* Remediation Recommendation: Remove the the superfluous deposits field.

### Notes
* Use of 'this.' way of calling functions in the same contract can cause both more gas and also unintended consequences such as msg.sender being equal to the contract itself
* Caching the state updates in some for loops could improve gas efficiency
* Comments in the Revenue sender stategy say it's for authorized senders but no authorization is found or nesecary
* https://github.com/FloorDAO/floor-v2/blob/74bec48f0dcdd08cdb2fcee89f24284a58124442/src/contracts/staking/VeFloorStaking.sol#L201 is more strict than necessary. I think instead the check could be currentEpoc + LOCK_PERIODS[epochs] >= depositor.epochStart + depositor.epochCount which allows transferring to shorter locks while redepositing if you retain at least your current lock on current deposits.
* I would recommend removing the set epoc function entirely as it has wide ranging bad consequences if used
* I would recommend removing ERC20 Permit imports from veFloor as the contract overrides and turns off approvals but dosen't remove permit which can still be used to approve somebody.
* veFloor has the rule that users cannot stake less than they already staked when depositing. This rule is not changed even if they fully withdraw as epochCount is not cleared on withdraw. Meaning if a user A stakes for 24 weeks and then withdraws at the end of the period and comes back 1 year later they cannot stake for 2 weeks.
* If any strategy contains self destruct [or had delegate call to a contract that did] it could result in the loss of access to funds.
* When you do (tokens * ((collectionVotePowers[i] * 1 ether) / totalRelevantVotes)) / 1 ether; I think it's simpler to do (tokens * collectionVotePowers[i] * 1 ) / totalRelevantVotes. [link](https://github.com/FloorDAO/floor-v2/blob/e159e937d21c0c09d53b9e4590f6b7a2317feb5a/src/contracts/voting/SweepWars.sol#L358)
* The onlyValidTokens modifier checks if tokens are in a list but is here used on a token loaded from that list and is therefore useless https://github.com/FloorDAO/floor-v2/blob/e7c19e38e21173ebd19eba9c1c0accb33a31bd26/src/contracts/strategies/DistributedRevenueStakingStrategy.sol#L77C76-L77C90
* I would recommend adding a way for the admin to remove strategies for the case that they begin locking in the snapshot function.