FungibleAssetVaultForDAO: direct value transfers

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L141 https://github.com/code-423n4/2022-04-jpegd/blob/main/contracts/vaults/FungibleAssetVaultForDAO.sol#L199

Vulnerability details

Impact

FungibleAssetVaultForDAO: deposit attempts to limit deposits to the whitelisted role, presumably because those are the only users which could later withdraw. This approach does help to communicate intentions well, but it does not stop someone from transferring in other ways - a direct ERC20 transfer or an ETH self destruct transfer. If funds were sent using one of those methods, they would be locked in the contract since withdraw uses the collateralAmount instead of the current balance (e.g. erc20.balanceOf(address(this))).

Proof of Concept

The contract accepts ERC20 tokens via a whitelisted transferFrom but no where in the contract is the ERC20.balanceOf(address(this)) considered and instead collateralAmount is used to track the expected balance.

Tools Used

Manual code review.

Recommended Mitigation Steps

You could switch to using the dynamic balance instead, functionally things would be the same but now if there is a direct transfer to the contract then those funds are acknowledged and can later be withdrawn.

[spaghettieth]

Duplicate of #39

[dmvt]

This is not a duplicate of #39. If someone manually sends funds to the contract directly and loses those funds, it is not really the problem of the protocol. Adding code to protect against users going far out of their own way to lose funds is not reasonable. Marking invalid.