Everything wrong with `deposit` method

[code423n4]

Lines of code

https://github.com/code-423n4/2023-03-aragon/blob/main/packages/contracts/src/core/dao/DAO.sol#L218-L245

Vulnerability details

Impact

No Accounting, Wrong data in Event emission in case of Fees on transfer Tokens and limited usecase.

Proof of Concept

File: DAO.sol

  function deposit(
        address _token,
        uint256 _amount,
        string calldata _reference
    ) external payable override {
        if (_amount == 0) revert ZeroAmount();

        if (_token == address(0)) {
            if (msg.value != _amount)
                revert NativeTokenDepositAmountMismatch({expected: _amount, actual: msg.value});
        } else {
            if (msg.value != 0)
                revert NativeTokenDepositAmountMismatch({expected: 0, actual: msg.value});

            IERC20Upgradeable(_token).safeTransferFrom(msg.sender, address(this), _amount);
        }

        emit Deposited(msg.sender, _token, _amount, _reference);
    }

Link to code

What this method does:

1. Anyone can deposit any amount of any token in the smart contract.
2. There is no accounting for any deposit made through this function.
3. It just emits an event with amount deposited, sender and token.

What's wrong about it:

1. In case of Fees on transfer token, the event emitted by it will give wrong information.
2. There is no method to take these tokens out in DAO.sol.
3. If it will seriously be used by DAO then without account it's use case are limit to just Donation.

Tools Used

VS Code

Recommended Mitigation Steps

Add accounting state variable to the users to track their funding amount. receive method can be removed so that only way to fund contract is througb deposit.

[c4-judge]

0xean marked the issue as unsatisfactory: Insufficient quality