code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Bank is a token vesting, airdrop and payroll tool. It uses merkle trees to massively scale token distributions with integrated vesting (time locks). The idea of this tool is that it allows DAOs to vest pre-sale participants, and future allocations of tokens (such as DAO treasury allocations) far into the future. These are important contracts since they need longevity and will secure large allocations of tokens.

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/README.md?plain=1#L28

Since these trees are long-lived, they need to be able to handle forks correctly. If someone generates an exchange address for their drops, that address may only be valid for that chain (e.g. exchange supports BTC but not BSV), and any funds sent to the unsupported chain are lost.

Impact

If there's a fork, since anyone can call withdraw(), an attacker can monitor the blockchain for calls to withdraw(), and then make the same call with the same arguments on the other chain, which will send funds to the unsupported address.

Proof of Concept

There are no EIP-712 protections in the encoding:

File: contracts/MerkleDropFactory.sol   #1

94           bytes32 leaf = keccak256(abi.encode(destination, value));

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

File: contracts/MerkleVesting.sol   #2

109           bytes32 leaf = keccak256(abi.encode(destination, totalCoins, startTime, endTime, lockPeriodEndTime));

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

and anyone can trigger a withdrawal:

File: contracts/MerkleDropFactory.sol   #3

82       /// @dev Anyone may call this function for anyone else, funds go to destination regardless, it's just a question of
83       /// @dev who provides the proof and pays the gas, msg.sender is not used in this function

https://github.com/code-423n4/2022-05-factorydao/blob/db415804c06143d8af6880bc4cda7222e5463c0e/contracts/MerkleDropFactory.sol#L82-L83

Tools Used

Code inspection

Recommended Mitigation Steps

Add EIP-712 protections and add a mechanism to allow tokens to be transferred to a different address using EIP-2612 permit()

illuzen commented 2 years ago

Valid