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.
illuzen
commented
2 years ago
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