Treasurey::getTotalValue
returns uint256 _totalValue
for accounting and transparency purposes. However uint256 _totalValue
does return a total cross-ERC20 token count and is therefor unusable and misleading.
Furthermore the function Treasury::allocateFunds
allocates a non ERC20 specific value towards something (most likely a proposal), lacks though the specification of which ERC20 is being allocated and therefore makes this function useless.
Treasury::deposit
:
As you can see here in the deposit function any user can deposit any ERC20 token into the Treasury Contract and uint256 _totalValue
will simply add the token amount, disregarding if the user actually deposits WBTC
or SHIB
(as example), making the value kind of useless at all.
Treasury::getTotalValue
:
Here the value will get returned for transparency reasons, making the impression by the naming that it actually returns a meaningful value, but as said above, the return value of this function is entirely meaningless.
Treasury::allocateFunds
:
Here is basically the same issue, non-ERC20 specific funds are being allocated (presumably towards a proposal(?)), but simply just allocated a random number of cross-ERC20 tokens. Without a specific token being allocated this function misses a crucial part for it's use case.
The impact of this confusing accounting mechanism is relatively straight forward.
Consider the following scenario:
100 Total Tokens in the contract, as an extreme example 2 wBTC and 98 SHIB tokens.
A governance user wants for example to form a proposal to spend some treasury funds on a charity donation, he proposes to spend 2% of the treasuries total value (the final value he would calculate from Treasury::getTotalValue
). At this point already the problem is quite obvious:
2% of the total value according to the function could be 2 wBTC, 1 wBTC and 1 SHIB or 2 SHIB which could barely be further away from each other in terms of value.
The same logic applies to the Treasury::allocateFunds
, nobody could remotely tell which part and what actual value is being allocated.
Furthermore the contract allows accounting for any ERC-20 tokens without whitelist via the deposit function, allowing users to deposit rebasing tokens, blacklist tokens and ERC20 tokens with malicious hooks, which all of them would be part of the accounting system of the treasury, while especially the last category, the treasury would not even be able to get rid of.
Likelihood: High
Impact: Low
Total Severity: Medium,
since it is influencing off-chain decision making and does not pose a risk towards the smart contract system itself, but the mentioned decision making could lead to misplaced spending proposals within the community and therefor indirectly harm the protocol.
Manual Review
The code below removes unnecessary accounting of uint256 _totalValue
and it's misleading properties. Users can still fetch significant balances via Treasury::getBalance
, so important functionality is still there. Also allocations would now be token specific and therefor meaningful.
The contest is live. Earn rewards by submitting a finding.
This is your time to appeal against judgements on your submissions.
Appeals are being carefully reviewed by our judges.