Summary
The FeeCollector contract sends RAAC tokens directly to the Treasury contract using transfer() in both FeeCollector::_processDistributions() and FeeCollector::emergencyWithdraw() functions. However, the Treasury contract only tracks token balances through its deposit() function. This mismatch causes tokens sent directly to Treasury to become permanently locked as they aren't tracked in the Treasury's internal accounting system, making it impossible to withdraw them.
Vulnerability Details
The issue occurs in two places in the FeeCollector contract:
In _processDistributions():
if (shares[3] > 0) raacToken.safeTransfer(treasury, shares[3]);
In emergencyWithdraw():
if (token == address(raacToken)) {
balance = raacToken.balanceOf(address(this));
raacToken.safeTransfer(treasury, balance);
}
The Treasury contract tracks balances using an internal mapping:
mapping(address => uint256) private _balances;
And requires this balance tracking for withdrawals:
function withdraw(address token, uint256 amount, address recipient) external {
if (_balances[token] < amount) revert InsufficientBalance();
_balances[token] -= amount;
}
When tokens are sent directly via transfer(), they bypass the Treasury's deposit() function which would have updated _balances. This causes a mismatch between the actual token balance and the tracked balance in the Treasury.
POC
To use foundry in the codebase, follow the hardhat guide here: Foundry-Hardhat hybrid integration by Nomic foundation
pragma solidity ^0.8.19;
import {FeeCollector} from "../../../../contracts/core/collectors/FeeCollector.sol";
import {Treasury} from "../../../../contracts/core/collectors/Treasury.sol";
import {RAACToken} from "../../../../contracts/core/tokens/RAACToken.sol";
import {veRAACToken} from "../../../../contracts/core/tokens/veRAACToken.sol";
import {Test, console} from "forge-std/Test.sol";
contract UnitTest is Test {
FeeCollector feeCollector;
Treasury treasury;
RAACToken raacToken;
veRAACToken veRAACTok;
address repairFund;
address admin;
uint256 initialSwapTaxRate = 100;
uint256 initialBurnTaxRate = 50;
function setUp() public {
repairFund = makeAddr("repairFund");
admin = makeAddr("admin");
treasury = new Treasury(admin);
raacToken = new RAACToken(admin, initialSwapTaxRate, initialBurnTaxRate);
veRAACTok = new veRAACToken(address(raacToken));
feeCollector = new FeeCollector(address(raacToken), address(veRAACTok), address(treasury), repairFund, admin);
vm.startPrank(admin);
raacToken.setFeeCollector(address(feeCollector));
raacToken.setMinter(admin);
vm.stopPrank();
}
function testTokensSentFromFeeCollectorGetsStuckInTreasury() public {
uint256 amount = feeCollector.MAX_FEE_AMOUNT();
uint256 amountToWithdraw = 1e18;
vm.startPrank(admin);
raacToken.mint(admin, amount);
raacToken.approve(address(feeCollector), amount);
feeCollector.collectFee(amount, 0);
feeCollector.pause();
feeCollector.emergencyWithdraw(address(raacToken));
vm.expectRevert();
treasury.withdraw(address(raacToken), amountToWithdraw, admin);
vm.stopPrank();
console.log("Balance of treasury: ", raacToken.balanceOf(address(treasury)));
console.log("Balance of Admin: ", raacToken.balanceOf(admin));
}
Impact
Any tokens sent to Treasury from FeeCollector become permanently locked. This affects both normal fee distribution and emergency withdrawals.
Tools Used
Manual review, foundry test suite
Recommendations
Modify FeeCollector to use Treasury's deposit function instead of direct transfers:
function _processDistributions(uint256 totalFees, uint256[4] memory shares) internal {
// ... existing code ...
- if (shares[3] > 0) raacToken.safeTransfer(treasury, shares[3]);
+ if (shares[3] > 0) {
+ raacToken.approve(treasury, shares[3]);
+ ITreasury(treasury).deposit(address(raacToken), shares[3]);
+ }
}
function emergencyWithdraw(address token) external whenPaused {
// ... existing code ...
if (token == address(raacToken)) {
balance = raacToken.balanceOf(address(this));
//@audit [h] would cause the tokens to get stuck in treasury contract as tokens are not tracked in `balances` over there when sent directly. This would cause the treasury withdraw function to always revert with overflow/underflow error because of the `_balances[token] -= amount;` check. Call deposit instead.
- raacToken.safeTransfer(treasury, balance);
+ raacToken.approve(treasury, balance);
+ ITreasury(treasury).deposit(token, balance);
}
}
