FactoryDAO contest - pedroais's results

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleVesting.sol#L173

Vulnerability details

Impact

Loss of funds

Proof of Concept

The ERC20 interface ensures a token transfer will return false on failure. In merkleVesting there is no requirement for this to be true. The contract doesn't ensure all the funds to cover the MerkleTree are present since it can't read the Merkle tree. However, if the funds aren't there yet the withdraw function should revert so users don't lose their claim if funds are added later.

If the token used doesn't revert to transfer failure a user could call the withdraw function and the transfer to his tokens could fail without reverting. The user claim will be set to true and he won't be able to claim again but he won't receive his tokens. This is done correctly in every transfer of the protocol except on this one.

Recommended Mitigation Steps

Require the transfer returns true

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L245 https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L224

Vulnerability details

Impact

Detailed description of the impact of this finding.

Proof of Concept

If an attacker makes many deposits of 1 wei the staking pool creator will have to make the withdraws himself to remove the unclaimed reward tokens. This can mean the pool creator will have to spend a huge amount of gas to do these withdrawals. https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L245

One could argue this attack won't happen since the attacker would also spend gas making the deposits. I argue the gas the attacker would spend could be much much less than the gas required to make the withdrawals if the rewards are distributed in many tokens. This happens because the withdraw function loops through every token and performs arithmetic to compute taxes for each token. This doesn't happen in the deposit function wich is of constant complexity. https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L224

An attacker can then force the owner to waste more gas than the excess rewards which would make the excess rewards not worth taking out. The attacker could do this without using much gas himself since the withdraw function is many times

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/PermissionlessBasicPoolFactory.sol#L316

Vulnerability details

Impact

Contract creator could steal all rewards using frontrunning

Proof of Concept

When a yield pool is created pool tax is set equal to global tax and funds are sent into the contract to pay for rewards. The contract creator could set tax to 100% in a transaction frontrunning a pool creation. He will then get all rewards distributed by that pool.

Tools Used

Recommended Mitigation Steps

Add a maximum global tax so it can't reach 100%. Also, frontrunning protection can be added. When a pool is created the creator could input the accepted tax as calldata. If the accepted tax isn't equal to the actual tax (for example because of frontrunning) pool creation should revert.

Lines of code

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleVesting.sol#L115

Vulnerability details

Impact

Loss of funds from multiple vestings for a single user

Proof of Concept

In MerkleVesting and MerkleResistor vestings are distributed using merkle trees. Creators of the vesting submit the Merkle root of the tree and deposit the funds to be distributed. A Merkle leaf in merkleVesting is defined by the following parameters : (destination, totalCoins, startTime, endTime, lockPeriodEndTime).

A tree could have many different leaves with the same destination but different vesting parameters. If that is done then users won't be able to claim their vesting since after claiming one initialized[destination][treeIndex] will be true.

If the tree creator sent the funds correctly then the user won't be able to claim his vestings and funds will be stuck inside the contract. https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleVesting.sol#L106

Recommended Mitigation Steps

Check the same leaf can't be used twice but not only with the destination. The mapping could be made to work with destination and start times or amounts. In that way the same user will be able to claim many vestings with different amounts / start times.