Submission Details
Severity:
Lack of Individual Tracking and Aggregated Global Balance Updates in `Treasury.sol` May Lead to Fund Misallocation and Accounting Inaccuracies
Summary
The `deposit()` and `withdraw()` in `Treasury.sol` rely on updating a global balance for each token without tracking individual user/contract deposits. In the deposit function, deposits are aggregated in a global `_balances` mapping, while the withdraw function deducts from this same global balance and updates a global `_totalValue` without any per-user differentiation. This repeated root cause can lead to significant misallocations during withdrawals, reward distributions, or fee calculations, resulting in potential fund mismanagement and accounting inaccuracies..
Vulnerability Details
In the deposit function, deposits are recorded by updating the _balances[token] variable:
```solidity
IERC20(token).transferFrom(msg.sender, address(this), amount);
_balances[token] += amount;
```
This approach aggregates deposits for each token across all users, which means that if multiple users deposit the same token, there is no record of individual contributions. Consequently, any process that later relies on these values (e.g., calculating rewards or handling withdrawals) cannot accurately determine each user’s share.
Withdraw Function
The withdraw function further perpetuates the issue by updating the same global balances:
```solidity
function withdraw(address token, uint256 amount, address recipient)
external
override
nonReentrant
onlyRole(MANAGER_ROLE)
{
if (token == address(0)) revert InvalidAddress();
if (recipient == address(0)) revert InvalidRecipient();
if (_balances[token] < amount) revert InsufficientBalance();
_balances[token] -= amount;
_totalValue -= amount;
IERC20(token).transfer(recipient, amount);
emit Withdrawn(token, amount, recipient);
}
```
Here, the function decreases the global balance `_balances[token]` and the global `_totalValue` without considering individual user contributions. The lack of per-user accounting in both the deposit and withdraw functions creates a systemic issue where deposits and withdrawals cannot be reconciled on a per-user basis.
### Proof of Concept
Imagine two users depositing tokens:
User A deposits 100 tokens.
User B deposits 50 tokens.
Both deposits update _balances[token] to 150 tokens without differentiating the users' contributions. When a withdrawal or reward distribution is triggered, the system has no way to correctly attribute the 100 tokens from User A and the 50 tokens from User B.
```solidity
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
import "forge-std/Test.sol";
import "../../../contracts/core/collectors/Treasury.sol";
import "lib/openzeppelin-contracts/contracts/mocks/token/ERC20Mock.sol";
contract MockERC20 is ERC20 {
constructor(string memory name, string memory symbol) ERC20(name, symbol) {
_mint(msg.sender, 100_000 * 10 ** 18); // Mint 100k tokens to deployer
}
}
contract TreasuryTest is Test {
Treasury treasury;
MockERC20 token;
address admin = address(0x1);
address manager = address(0x2);
address allocator = address(0x3);
address user = address(0x7);
address userA = address(0x4);
address userB = address(0x6);
address recipient = address(0x5);
// Role hashes
bytes32 constant MANAGER_ROLE = keccak256("MANAGER_ROLE");
bytes32 constant ALLOCATOR_ROLE = keccak256("ALLOCATOR_ROLE");
function setUp() public {
// Deploy mock token
token = new MockERC20("Mock Token", "MTK");
// Deploy Treasury contract with admin
vm.prank(admin);
treasury = new Treasury(admin);
// Grant roles
vm.startPrank(admin);
treasury.grantRole(MANAGER_ROLE, manager);
treasury.grantRole(ALLOCATOR_ROLE, allocator);
vm.stopPrank();
// Transfer some tokens to user for testing deposits
token.transfer(userA, 10_000 * 10 ** 18);
token.transfer(userB, 10_000 * 10 ** 18);
}
// Test deposit functionality
function testBalanceAfterMultipleDeposits() public {
uint256 userADepositAmount = 1_00 * 10 ** 18;
uint256 userBDepositAmount = 1_00 * 10 ** 18;
// Approve and deposit from user
vm.startPrank(userA);
token.approve(address(treasury), userADepositAmount);
treasury.deposit(address(token), userADepositAmount);
vm.stopPrank();
vm.startPrank(userB);
token.approve(address(treasury), userBDepositAmount);
treasury.deposit(address(token), userBDepositAmount);
vm.stopPrank();
// Assertions
assertEq(
treasury.getBalance(address(token)),
userADepositAmount,
"get balance is returning total balance of the pool"
);
}
}
```
the result
```solidity
forge t --mt testBalanceAfterMultipleDeposits -vv
[⠊] Compiling...
No files changed, compilation skipped
Ran 1 test for test/foundry/Collectors/TreasuryTest.t.sol:TreasuryTest
[FAIL: get balance is returning total balance of the pool: 200000000000000000000 != 100000000000000000000] testBalanceAfterMultipleDeposits() (gas: 177384)
Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 1.71ms (427.26µs CPU time)
Ran 1 test suite in 11.92ms (1.71ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)
Failing tests:
Encountered 1 failing test in test/foundry/Collectors/TreasuryTest.t.sol:TreasuryTest
[FAIL: get balance is returning total balance of the pool: 200000000000000000000 != 100000000000000000000] testBalanceAfterMultipleDeposits() (gas: 177384)
Encountered a total of 1 failing tests, 0 tests succeeded
```
Impact
Misallocation of Funds: Without per-user tracking, subsequent processes (like withdrawals, rewards, or fee calculations) cannot accurately determine the rightful owner of each deposit.
Exploitation Risks: A malicious actor might exploit the lack of individual records to manipulate fund allocation or interfere with the proper operation of the protocol.
Accounting Inaccuracies: Aggregating deposits in this manner can lead to discrepancies in user balances, undermining the integrity of the system.
Tools Used
Manual Review and foundry
Recommendations
Implement Per-User Tracking:
Introduce a mapping that tracks deposits per user per token. For example:
```solidity
mapping(address => mapping(address => uint256)) private _userBalances;
```
Refactor the Deposit Function:
Update the function to record deposits on a per-user basis:
```solidity
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);
_userBalances[msg.sender][token] += amount;
_balances[token] += amount; // optional: keep token totals if needed
emit Deposited(token, amount);
}
```
Refactor Withdraw Function to Consider Individual User Balances:
Modify the withdraw function so that it either operates on per-user balances or includes logic to ensure that the deduction of funds is correctly attributed:
```solidity
function withdraw(address token, uint256 amount, address recipient)
external
override
nonReentrant
onlyRole(MANAGER_ROLE)
{
if (token == address(0)) revert InvalidAddress();
if (recipient == address(0)) revert InvalidRecipient();
// For user-specific withdrawals, implement checks against _userBalances
// Alternatively, ensure that global deductions are reconciled with individual deposits
if (_balances[token] < amount) revert InsufficientBalance();
_balances[token] -= amount;
_totalValue -= amount;
IERC20(token).transfer(recipient, amount);
emit Withdrawn(token, amount, recipient);
}
```
By implementing per-user tracking in both the deposit and withdraw functions, the contract can ensure accurate accounting and proper allocation of funds, mitigating the risk of mismanagement and potential exploitation.
Updates
Lead Judging Commences
inallhonesty 3 months ago
Submission Judgement Published Assigned finding tags:
Treasury::deposit increments _totalValue regardless of the token, be it malicious, different decimals, FoT etc.
Prize pool breakdown
Total prize
77,280 USDC
nSLOC
3,86420 USDC / LOC
74,000
3,280
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.