Core Contracts

Summary

_totalValue is supposed to be the "Total value across all tokens" (see comment in the code). Given that the Treasury contract can receive different tokens, this variable should store the total value in USD of all tokens to have any meaning.

The problem is that currently, _totalValue is incremented with _totalValue += amount; in deposit function, and decremented in withdraw function. Given that deposit function is public and can receive any token, an attacker could easily manipulate the value of _totalValue variable, potentially setting it to the max uint256 value. This would lead to DoS of deposit function because of overflow error.

Vulnerability Details

The depositfunction is defined as follows:

The probleme arises because _totalValue is simply incremented by the amount of token deposited, no matter what token it is and what its price is.

Consequently, an attacker can create an ERC20 token and mint himself type(uint256).max tokens if the treasury contract is empty, or the right amount (type(uint256).max - _totalValue) if the treasury already collected fees. That way, he can call deposit and transfer all his tokens to the treasury, with _totalValue being updated to type(uint256).max.

After that, any call to deposit will fail with an overflow error on _totalValue += amount.

Besides, getTotalValue external function will return a wrong result (total amount of tokens no matter what token, instead of total value of all the tokens received through deposit function).

Note that currently, FeeCollector sends fees to the treasury through direct tokens transfers, without using deposit function (other issue).

Impact

The impact of this vulnerability is high, given that it leads to permanent impossibility to deposit funds in the treasury through deposit function, making it unusable.

Tools Used

Manual review.

Recommendations

One possible solution would to be to make sure that _totalValue is incremented by the value in USD of the tokens received, using an oracle. That way, _totalValue will never reach type(uint256).max and no DoS attack will be possible.