Core Contracts
Regnum Aurum Acquisition Corp
HardhatReal World AssetsNFT
77,280 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

Treasury.sol has misleading accounting, leading to uninformed government proposals

0xtimefliez

Description

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.

Vulnerable Code

Treasury::deposit:

function deposit(address token, uint256 amount) external override nonReentrant {
if (token == address(0)) revert InvalidAddress();
if (amount == 0) revert InvalidAmount();
IERC20(token).transferFrom(msg.sender, address(this), amount);
_balances[token] += amount;
@> _totalValue += amount;
emit Deposited(token, amount);
}

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:

function getTotalValue() external view override returns (uint256) {
return _totalValue;
}

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:

function allocateFunds(
address recipient,
uint256 amount
) external override onlyRole(ALLOCATOR_ROLE) {
if (recipient == address(0)) revert InvalidRecipient();
if (amount == 0) revert InvalidAmount();
_allocations[msg.sender][recipient] = amount;
emit FundsAllocated(recipient, amount);
}

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.

Impact

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.

Tools Used

Manual Review

Recommended Fix

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.

contract Treasury is ITreasury, AccessControl, ReentrancyGuard {
bytes32 public constant MANAGER_ROLE = keccak256("MANAGER_ROLE");
bytes32 public constant ALLOCATOR_ROLE = keccak256("ALLOCATOR_ROLE");
mapping(address => uint256) private _balances;
- mapping(address => mapping(address => uint256)) private _allocations;
+ mapping(address => mapping(address => mapping(address => uint256))) private _allocations;
- uint256 private _totalValue;
​
/// **SNIP** ///
​
function deposit(address token, uint256 amount) external override nonReentrant {
if (token == address(0)) revert InvalidAddress();
if (amount == 0) revert InvalidAmount();
IERC20(token).transferFrom(msg.sender, address(this), amount);
_balances[token] += amount;
- _totalValue += amount;
emit Deposited(token, amount);
}
​
​
function allocateFunds(
address recipient,
uint256 amount,
address token
) external override onlyRole(ALLOCATOR_ROLE) {
if (recipient == address(0)) revert InvalidRecipient();
if (amount == 0) revert InvalidAmount();
- _allocations[msg.sender][recipient] = amount;
- emit FundsAllocated(recipient, amount);
+ _allocations[msg.sender][recipient][token] = amount;
+ emit FundsAllocated(recipient, amount, token);
}
​
- function getTotalValue() external view override returns (uint256) {
- return _totalValue;
- }
​
/// **SNIP** ///
​
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 month ago
Submission Judgement Published
Validated
Assigned finding tags:

Treasury::deposit increments _totalValue regardless of the token, be it malicious, different decimals, FoT etc.

Treasury::allocateFunds doesn't say what token you are actually allocating, doesn't check balances, or existing allocations to other recipients

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.