Submission Details
Severity:
Direct transfer of tokens from FeeCollector to Treasury locks the tokens permanently inside the Treasury contract
Summary
FeeCollector contract has some functions which transfer tokens from its address to the address of the treasury. Tokens should be deposited into the Treasury contract by calling its deposit function, otherwise they can't be withdrawn from that contract and are locked forever.
Vulnerability Details
FeeCollector::_processDistributions() and FeeCollector::emergencyWithdraw() functions use the safeTransfer function to transfer tokens to the treasury address, at lines 276, 279, 423 of FeeCollector.sol.
If the treasury address refers to the Treasury contract, those direct transfer operations just lock those tokens inside the Treasury contract, as Treasury::_balances and Treasury::_totalValue are not updated, whose updation is required in order to withdraw those tokens using the Treasury::withdraw() function.
So FeeCollector should call Treasury::deposit() whenever it sends some tokens to that contract.
But it's also true that FeeCollector::treasury address can be an externally owned account (as per the test files), or some other contract, or a multi-sig wallet. In those cases, Treasury::deposit() shouldn't and can't be called.
Impact
One contract used by the protocol permanently locks tokens inside another contract used by the protocol, making those tokens irrecoverable.
Proof of Concept
I created a new test file, copying much of the initialization code and some test cases from FeeCollector.test.js. I added some lines where Treasury::withdraw() is called.
As per the current implementation of contracts, these test cases would fail:
// test/unit/core/collectors/FeeCollectorTreasury.test.js
import { expect } from "chai";
import hre from "hardhat";
const { ethers } = hre;
import { time } from "@nomicfoundation/hardhat-network-helpers";
describe("FeeCollector & Treasury", function () {
let raacToken, feeCollector, treasury, veRAACToken, testToken;
let owner, user1, repairFund, emergencyAdmin;
let defaultFeeType;
const BASIS_POINTS = 10000;
const WEEK = 7 * 24 * 3600;
const ONE_YEAR = 365 * 24 * 3600;
const INITIAL_MINT = ethers.parseEther("10000");
const SWAP_TAX_RATE = 100; // 1%
const BURN_TAX_RATE = 50; // 0.5%
beforeEach(async function () {
[owner, user1, repairFund, emergencyAdmin] = await ethers.getSigners();
// Deploy RAACToken
const RAACToken = await ethers.getContractFactory("RAACToken");
raacToken = await RAACToken.deploy(owner.address, SWAP_TAX_RATE, BURN_TAX_RATE);
await raacToken.waitForDeployment();
// Deploy veRAACToken
const VeRAACToken = await ethers.getContractFactory("veRAACToken");
veRAACToken = await VeRAACToken.deploy(await raacToken.getAddress());
await veRAACToken.waitForDeployment();
// Deploy Treasury
const Treasury = await ethers.getContractFactory("Treasury");
treasury = await Treasury.deploy(owner.address);
await treasury.waitForDeployment();
// Deploy FeeCollector
const FeeCollector = await ethers.getContractFactory("FeeCollector");
feeCollector = await FeeCollector.deploy(
await raacToken.getAddress(),
await veRAACToken.getAddress(),
await treasury.getAddress(),
repairFund.address,
owner.address
);
await feeCollector.waitForDeployment();
// Setup initial configuration
await raacToken.setFeeCollector(await feeCollector.getAddress());
await raacToken.manageWhitelist(await feeCollector.getAddress(), true);
await raacToken.manageWhitelist(await veRAACToken.getAddress(), true);
await raacToken.setMinter(owner.address);
await veRAACToken.setMinter(owner.address);
// Setup roles
await feeCollector.grantRole(await feeCollector.FEE_MANAGER_ROLE(), owner.address);
await feeCollector.grantRole(await feeCollector.EMERGENCY_ROLE(), emergencyAdmin.address);
await feeCollector.grantRole(await feeCollector.DISTRIBUTOR_ROLE(), owner.address);
// Mint initial tokens and approve
await raacToken.mint(user1.address, INITIAL_MINT);
await raacToken.connect(user1).approve(await feeCollector.getAddress(), ethers.MaxUint256);
await raacToken.connect(user1).approve(await veRAACToken.getAddress(), ethers.MaxUint256);
// Setup initial fee types
defaultFeeType = {
veRAACShare: 5000, // 50%
burnShare: 1000, // 10%
repairShare: 1000, // 10%
treasuryShare: 3000 // 30%
};
for (let i = 0; i < 8; i++) {
await feeCollector.connect(owner).updateFeeType(i, defaultFeeType);
}
// Calculate gross amounts needed including transfer tax
const taxRate = SWAP_TAX_RATE + BURN_TAX_RATE; // 150 basis points (1.5%)
const grossMultiplier = BigInt(BASIS_POINTS * BASIS_POINTS) / BigInt(BASIS_POINTS * BASIS_POINTS - taxRate * BASIS_POINTS);
const protocolFeeGross = ethers.parseEther("50") * grossMultiplier / BigInt(10000);
const lendingFeeGross = ethers.parseEther("30") * grossMultiplier / BigInt(10000);
const swapTaxGross = ethers.parseEther("20") * grossMultiplier / BigInt(10000);
// Collect fees
await feeCollector.connect(user1).collectFee(protocolFeeGross, 0);
await feeCollector.connect(user1).collectFee(lendingFeeGross, 1);
await feeCollector.connect(user1).collectFee(swapTaxGross, 6);
// Create veRAACToken locks
await veRAACToken.connect(user1).lock(ethers.parseEther("1000"), ONE_YEAR);
await time.increase(WEEK);
});
describe("Fee Collection and Distribution", function () {
it("should distribute fees correctly between stakeholders and recover from treasury", async function () {
// Get initial balances
const initialTreasuryBalance = await raacToken.balanceOf(treasury.target);
const initialRepairFundBalance = await raacToken.balanceOf(repairFund.address);
const initialUserBalance = await raacToken.balanceOf(user1.address);
// Calculate net amounts after initial transfer tax (1.5%)
const taxRate = SWAP_TAX_RATE + BURN_TAX_RATE; // 150 basis points
const netMultiplier = BigInt(BASIS_POINTS - taxRate) / BigInt(BASIS_POINTS);
const protocolFeesNet = ethers.parseEther("50") * netMultiplier;
const lendingFeesNet = ethers.parseEther("30") * netMultiplier;
const swapTaxesNet = ethers.parseEther("20") * netMultiplier;
// Calculate shares based on fee types (using verified percentages)
const treasuryShare = (
(protocolFeesNet * 3000n) / 10000n + // 30% of protocol fees
(lendingFeesNet * 3000n) / 10000n // 30% of lending fees
);
const repairShare = (
(protocolFeesNet * 1000n) / 10000n + // 10% of protocol fees
(swapTaxesNet * 1000n) / 10000n // 10% of swap taxes
);
// Apply second transfer tax
const treasuryShareNet = treasuryShare * netMultiplier;
const repairShareNet = repairShare * netMultiplier;
// Distribute fees
await feeCollector.connect(owner).distributeCollectedFees();
// Get final balances
const finalTreasuryBalance = await raacToken.balanceOf(treasury.target);
const finalRepairFundBalance = await raacToken.balanceOf(repairFund.address);
// Use a smaller margin for comparison
const margin = ethers.parseEther("0.01");
expect(finalTreasuryBalance).to.be.closeTo(
initialTreasuryBalance + treasuryShareNet,
margin
);
expect(finalRepairFundBalance).to.be.closeTo(
initialRepairFundBalance + repairShareNet,
margin
);
// Withdraw from treasury
await expect(treasury.connect(owner).withdraw(
await raacToken.getAddress(),
finalTreasuryBalance,
user1.address
)).to.be.fulfilled;
// Treasury balance should go back to its initial balance, user1's balance should increase
expect(await raacToken.balanceOf(treasury.target)).to.equal(initialTreasuryBalance);
expect(await raacToken.balanceOf(user1.address)).to.be.greaterThan(initialUserBalance);
});
});
describe("Emergency Controls", function () {
beforeEach(async function () {
await feeCollector.connect(owner).grantRole(await feeCollector.EMERGENCY_ROLE(), owner.address);
await raacToken.connect(user1).transfer(feeCollector.target, ethers.parseEther("100"));
});
it("should allow emergency withdrawal and recovery of RAACToken by admin", async function () {
await feeCollector.connect(owner).pause();
const amount = ethers.parseEther("100");
// Calculate expected amount after two tax applications
const taxRate = SWAP_TAX_RATE + BURN_TAX_RATE; // 150 basis points (1.5%)
const firstTaxAmount = amount * BigInt(taxRate) / BigInt(10000);
const remainingAfterFirstTax = amount - firstTaxAmount;
const secondTaxAmount = remainingAfterFirstTax * BigInt(taxRate) / BigInt(10000);
const expectedAmount = remainingAfterFirstTax - secondTaxAmount;
const initialBalance = await raacToken.balanceOf(feeCollector.target);
expect(initialBalance).to.equal(100010000000000000000n);
await raacToken.connect(user1).transfer(feeCollector.target, amount);
const newBalance = await raacToken.balanceOf(feeCollector.target);
expect(newBalance).to.equal(200010000000000000000n);
const treasuryBalanceBeforeEmergencyWithdrawal = await raacToken.balanceOf(treasury.target);
await feeCollector.connect(owner).emergencyWithdraw(raacToken.target);
const finalTreasuryBalance = await raacToken.balanceOf(treasury.target);
// The entire balance should be transferred to treasury
expect(finalTreasuryBalance).to.equal(200010000000000000000n);
// Withdraw from treasury to "recovery account" - user1
const userBalanceBeforeRecovery = await raacToken.balanceOf(user1.address);
await expect(treasury.connect(owner).withdraw(
await raacToken.getAddress(),
finalTreasuryBalance,
user1.address
)).to.be.fulfilled;
// Treasury balance should go back to its initial balance, user1's balance should increase
expect(await raacToken.balanceOf(treasury.target)).to.equal(treasuryBalanceBeforeEmergencyWithdrawal);
expect(await raacToken.balanceOf(user1.address)).to.be.greaterThan(userBalanceBeforeRecovery);
});
it("should allow emergency withdrawal and recovery of any test token by admin", async function () {
await feeCollector.connect(owner).pause();
// Deploy mock token using pattern from MockToken.sol
const MockToken = await ethers.getContractFactory("MockToken");
testToken = await MockToken.deploy("Test Token", "TEST", 18);
const initUserBalance = ethers.parseEther("1000");
await testToken.mint(user1.address, initUserBalance);
const amount = ethers.parseEther("100");
// Transfer test tokens to fee collector's address
await testToken.connect(user1).transfer(feeCollector.target, amount);
expect(await testToken.balanceOf(user1)).to.equal(initUserBalance - amount);
expect(await testToken.balanceOf(feeCollector.target)).to.equal(amount);
expect(await testToken.balanceOf(treasury.target)).to.equal(0);
expect(await treasury.getTotalValue()).to.equal(0);
expect(await treasury.getBalance(testToken.target)).to.equal(0);
// Emergency withdraw test tokens from fee collector
await feeCollector.connect(owner).emergencyWithdraw(testToken.target);
expect(await testToken.balanceOf(feeCollector.target)).to.equal(0);
expect(await testToken.balanceOf(treasury.target)).to.equal(amount);
expect(await treasury.getTotalValue()).to.equal(amount);
expect(await treasury.getBalance(testToken.target)).to.equal(amount);
// Withdraw funds from treasury
await expect(treasury.connect(owner).withdraw(
testToken.target,
amount,
user1.address
)).to.be.fulfilled;
expect(await testToken.balanceOf(user1)).to.equal(initUserBalance);
expect(await testToken.balanceOf(feeCollector.target)).to.equal(0);
expect(await testToken.balanceOf(treasury.target)).to.equal(0);
expect(await treasury.getTotalValue()).to.equal(0);
expect(await treasury.getBalance(testToken.target)).to.equal(0);
});
});
});
Tools Used
Manual review
Recommendations
FeeCollector should call Treasury::deposit() conditionally:
If
FeeCollector::treasuryis an externally owned account or a multisig wallet or any other contract which doesn't require/have a custom deposit logic,safeTransfer()should be used.If
FeeCollector::treasuryis theTreasurycontract,Treasury::deposit()should be used after approving the transfer amount to the treasury address.
For this, the Treasury contract should contain a function which tells the caller that it supports a custom deposit logic. FeeCollector should rely on the result of this function and make a call to deposit().
The following changes can be applied to the contracts, after which the above test cases as well as the existing test files FeeCollector.test.js and Treasury.test.js would pass:
diff --git a/contracts/core/collectors/FeeCollector.sol b/contracts/core/collectors/FeeCollector.sol
index 4e93083..c99fe46 100644
--- a/contracts/core/collectors/FeeCollector.sol
+++ b/contracts/core/collectors/FeeCollector.sol
import "@openzeppelin/contracts/access/AccessControl.sol";
import "@openzeppelin/contracts/utils/Pausable.sol";
import "../../interfaces/core/collectors/IFeeCollector.sol";
+import "../../interfaces/core/collectors/ITreasury.sol";
import "../../interfaces/core/tokens/IveRAACToken.sol";
import "../../interfaces/core/tokens/IRAACToken.sol";
contract FeeCollector is IFeeCollector, AccessControl, ReentrancyGuard, Pausable
if (!hasRole(EMERGENCY_ROLE, msg.sender)) revert UnauthorizedCaller();
if (token == address(0)) revert InvalidAddress();
- uint256 balance;
- if (token == address(raacToken)) {
- balance = raacToken.balanceOf(address(this));
- raacToken.safeTransfer(treasury, balance);
- } else {
- balance = IERC20(token).balanceOf(address(this));
- SafeERC20.safeTransfer(IERC20(token), treasury, balance);
- }
+ uint256 balance = IERC20(token).balanceOf(address(this));
+ _depositTokensToTreasury(token, balance);
emit EmergencyWithdrawal(token, balance);
}
contract FeeCollector is IFeeCollector, AccessControl, ReentrancyGuard, Pausable
if (shares[1] > 0) raacToken.burn(shares[1]);
if (shares[2] > 0) raacToken.safeTransfer(repairFund, shares[2]);
- if (shares[3] > 0) raacToken.safeTransfer(treasury, shares[3]);
+ if (shares[3] > 0) _depositTokensToTreasury(address(raacToken), shares[3]);
}
/**
contract FeeCollector is IFeeCollector, AccessControl, ReentrancyGuard, Pausable
return 0;
}
+ /**
+ * @dev Deposits tokens to treasury address. If it is the dedicated
+ * treasury contract, calls deposit(). If not, just transfers the tokens.
+ * @param _token token address
+ * @param _amount amount of tokens to send
+ */
+ function _depositTokensToTreasury(address _token, uint256 _amount) internal {
+ address to = treasury;
+ bool _isTreasury = _isTreasuryContract(to);
+ if (_isTreasury) {
+ SafeERC20.forceApprove(IERC20(_token), to, _amount);
+ ITreasury(to).deposit(_token, _amount);
+ } else {
+ SafeERC20.safeTransfer(IERC20(_token), to, _amount);
+ }
+ }
+
+ /**
+ * @notice Tells if the supplied address is treasury contract
+ * @param _treasury the address
+ * @return bool true if the address is a treasury contract.
+ * false if it's an EOA or some other contract, e.g. a multisig wallet.
+ */
+ function _isTreasuryContract(address _treasury) internal view returns (bool) {
+ uint256 _size;
+ assembly {
+ _size := extcodesize(_treasury)
+ }
+ if (_size > 0) {
+ try ITreasury(_treasury).hasCustomDeposit() returns (bool _isTreasury) {
+ return _isTreasury;
+ } catch { }
+ }
+ return false;
+ }
+
// View Functions
/**
diff --git a/contracts/core/collectors/Treasury.sol b/contracts/core/collectors/Treasury.sol
index fc8cad6..5a25d38 100644
--- a/contracts/core/collectors/Treasury.sol
+++ b/contracts/core/collectors/Treasury.sol
contract Treasury is ITreasury, AccessControl, ReentrancyGuard {
function getAllocation(address allocator, address recipient) external view returns (uint256) {
return _allocations[allocator][recipient];
}
+
+ /**
+ * @notice Tells the caller that this contract has custom token deposit strategy
+ * so that the caller should call deposit() instead of directly transferring
+ * their tokens.
+ * @return bool true
+ */
+ function hasCustomDeposit() external pure returns (bool) {
+ return true;
+ }
}
diff --git a/contracts/interfaces/core/collectors/ITreasury.sol b/contracts/interfaces/core/collectors/ITreasury.sol
index 107192a..7353d14 100644
--- a/contracts/interfaces/core/collectors/ITreasury.sol
+++ b/contracts/interfaces/core/collectors/ITreasury.sol
interface ITreasury {
*/
function getAllocation(address allocator, address recipient) external view returns (uint256);
+ /**
+ * @notice Tells the caller that this contract has custom token deposit strategy
+ * so that the caller should call deposit() instead of directly transferring
+ * their tokens.
+ * @return bool true
+ */
+ function hasCustomDeposit() external view returns (bool);
+
/**
* @notice Events
*/
diff --git a/contracts/mocks/core/collectors/MockTreasury.sol b/contracts/mocks/core/collectors/MockTreasury.sol
index 5ef4ca0..b356896 100644
--- a/contracts/mocks/core/collectors/MockTreasury.sol
+++ b/contracts/mocks/core/collectors/MockTreasury.sol
contract MockTreasury is ITreasury {
function mock_setAllocation(address allocator, address recipient, uint256 amount) external {
_allocations[allocator][recipient] = amount;
}
+
+ function hasCustomDeposit() external pure returns (bool) {
+ return true;
+ }
}
Updates
Lead Judging Commences
inallhonesty 3 months ago
Submission Judgement Published Assigned finding tags:
FeeCollector::_processDistributions and emergencyWithdraw directly transfer funds to Treasury where they get permanently stuck
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.