Submission Details
Severity:
Global Total Value Update Ignores Token Specificity, Leading to Inaccurate Accounting
Summary
The deposit function in the `Treasury.sol` updates a global variable _totalValue with the deposited amount regardless of the token type. This approach aggregates deposits from different tokens into a single total, potentially causing miscalculations in the overall system value and misallocation of funds.
Vulnerability Details
Within the `deposit()` in the `Treasury.sol`, after processing the deposit, the code updates the global total:
```solidity
_totalValue += amount;
```
This update does not account for the fact that different tokens may have different values or decimal representations. Aggregating all deposits into a single `_totalValue` can be misleading when the tokens involved are not homogeneous in value.
### Proof of Concept
Consider the following scenario:
User A deposits 100 units of Token X (high value).
User B deposits 100 units of Token Y (low value).
Both deposits are summed into `_totalValue` without differentiation. Subsequent calculations that rely on `_totalValue` will treat both tokens equally, potentially causing errors in value-based distributions.
```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);
}
function testTotalValueMisrepresentsTokenValue() public {
// Deploy two tokens with same decimals but implied different values
MockERC20 tokenX = new MockERC20("Token X", "TKX"); // High-value token, e.g., $100/unit
MockERC20 tokenY = new MockERC20("Token Y", "TKY"); // Low-value token, e.g., $0.01/unit
// Transfer tokens to users
tokenX.transfer(userA, 1_000 * 10**18); // 1000 units of Token X
tokenY.transfer(userB, 1_000 * 10**18); // 1000 units of Token Y
// User A deposits 100 units of Token X
uint256 depositAmountX = 100 * 10**18; // 100 Token X
vm.startPrank(userA);
tokenX.approve(address(treasury), depositAmountX);
treasury.deposit(address(tokenX), depositAmountX);
vm.stopPrank();
// User B deposits 100 units of Token Y
uint256 depositAmountY = 100 * 10**18; // 100 Token Y
vm.startPrank(userB);
tokenY.approve(address(treasury), depositAmountY);
treasury.deposit(address(tokenY), depositAmountY);
vm.stopPrank();
// Check individual balances
assertEq(treasury.getBalance(address(tokenX)), depositAmountX, "Token X balance incorrect");
assertEq(treasury.getBalance(address(tokenY)), depositAmountY, "Token Y balance incorrect");
// Check totalValue
uint256 actualTotalValue = treasury.getTotalValue();
uint256 rawSum = depositAmountX + depositAmountY; // 100 * 10^18 + 100 * 10^18 = 200 * 10^18
// Log values for inspection
console.log("Token X Deposited (high value):", depositAmountX / 10**18, "units");
console.log("Token Y Deposited (low value):", depositAmountY / 10**18, "units");
console.log("Raw Total Value:", actualTotalValue / 10**18, "units");
// Assertion matches current logic but highlights the issue
assertEq(actualTotalValue, rawSum, "Total value matches raw sum but misrepresents real value");
// Commentary: In reality, if Token X = $100/unit and Token Y = $0.01/unit:
// - 100 Token X = $10,000
// - 100 Token Y = $1
// - Real total value = $10,001, but _totalValue = 200 units, which is misleading
console.log("Note: Total Value treats 100 high-value Token X and 100 low-value Token Y as equal (200 units)");
}
}
```
the result
```solidity
Ran 1 test for test/foundry/Collectors/TreasuryTest.t.sol:TreasuryTest
[PASS] testTotalValueMisrepresentsTokenValue() (gas: 1905432)
Logs:
Token X Deposited (high value): 100 units
Token Y Deposited (low value): 100 units
Raw Total Value: 200 units
Note: Total Value treats 100 high-value Token X and 100 low-value Token Y as equal (200 units)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.00ms (742.13µs CPU time)
Ran 1 test suite in 11.38ms (2.00ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)
```
Impact
Inaccurate Accounting: Combining deposits from tokens with different valuations can lead to incorrect computations of the system’s total value.
Misallocation of Resources: Future operations relying on _totalValue (e.g., fee calculations, reward distributions) may be skewed due to the mixed valuation of tokens.
Potential Exploitation: An attacker might exploit these miscalculations to manipulate fee or reward distributions, compromising the protocol’s financial integrity.
Tools Used
Manual Review and Foundry
Recommendations
Token-Specific Total Accounting:
Instead of using a single global _totalValue, maintain a per-token total with a mapping:
```solidity
mapping(address => uint256) private _tokenTotalValue;
```
Refactor the Deposit Function:
Update the function to account for each token individually:
```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);
_balances[token] += amount;
_tokenTotalValue[token] += amount;
emit Deposited(token, amount);
}
```
This change ensures that each token's total value is tracked separately, leading to more accurate accounting and reducing the risk of miscalculations in further financial operations.
Updates
Lead Judging Commences
inallhonesty 6 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.