Summary
The FeeCollector contract can be paused and any funds can be swept to the Treasury contract in case of emergency.
The issue is the Treasury contract tracks balances only if deposit() is explicitly called.
In FeeCollector::emergencyWithdraw(), the funds are transferred directly to the Treasury address instead of calling deposit().
This results in the emergency funds which are swept from the FeeCollector to be permanently stuck in the Treasury contract because withdraw() will revert.
Vulnerability Details
function emergencyWithdraw(address token) external override whenPaused {
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);
}
emit EmergencyWithdrawal(token, balance);
}
In either scenario, tokens are transferred directly to the Treasury address.
function deposit(address token, uint256 amount) external override nonReentrant {
IERC20(token).transferFrom(msg.sender, address(this), amount);
> _balances[token] += amount;
_totalValue += amount;
emit Deposited(token, amount);
}
function withdraw(
address token,
uint256 amount,
address recipient
) external override nonReentrant onlyRole(MANAGER_ROLE) {
> if (_balances[token] < amount) revert InsufficientBalance();
_balances[token] -= amount;
_totalValue -= amount;
IERC20(token).transfer(recipient, amount);
emit Withdrawn(token, amount, recipient);
}
Any tokens that are transferred to the Treasury contract without calling deposit() are lost because withdrawals are only allowed up to the value being tracked in the _balances mapping.
Add the below test to test/e2e/protocol-tests.js
Run with the following command: npm run test:e2e
it('emergency withdraw to treasury results in stuck tokens', async function () {
const initialFeeCollectorBalance = await contracts.raacToken.balanceOf(contracts.feeCollector.target);
const initialUser1Balance = await contracts.raacToken.balanceOf(user2.address);
await contracts.raacToken.connect(user2).transfer(contracts.feeCollector.target, TRANSFER_AMOUNT);
const afterFeeCollectorBalance = await contracts.raacToken.balanceOf(contracts.feeCollector.target);
expect(afterFeeCollectorBalance).to.be.gt(initialFeeCollectorBalance);
await contracts.feeCollector.connect(owner).pause();
await contracts.feeCollector.connect(owner).emergencyWithdraw(contracts.raacToken.target);
const finalFeeCollectorBalance = await contracts.raacToken.balanceOf(contracts.feeCollector.target);
expect(finalFeeCollectorBalance).to.be.lt(afterFeeCollectorBalance);
const trackedTreasuryRaacBalance = await contracts.treasury.getBalance(contracts.raacToken.target);
console.log(`${trackedTreasuryRaacBalance}`);
const actualTreasuryRaacBalance = await contracts.raacToken.balanceOf(contracts.treasury.target);
console.log(`${actualTreasuryRaacBalance}`);
await contracts.treasury.connect(owner).withdraw(contracts.raacToken.target, TRANSFER_AMOUNT, user1);
});
0
102000000000000000000
1) emergency withdraw to treasury results in stuck tokens
1) Protocol E2E Tests
RAAC Token
emergency withdraw to treasury results in stuck tokens:
Error: VM Exception while processing transaction: reverted with custom error 'InsufficientBalance()'
at Treasury.withdraw (contracts/core/collectors/Treasury.sol:71)
at EdrProviderWrapper.request (node_modules/hardhat/src/internal/hardhat-network/provider/provider.ts:398:41)
at HardhatEthersSigner.sendTransaction (node_modules/@nomicfoundation/hardhat-ethers/src/signers.ts:125:18)
at send (node_modules/ethers/src.ts/contract/contract.ts:313:20)
at Proxy.withdraw (node_modules/ethers/src.ts/contract/contract.ts:352:16)
at Context.<anonymous> (file:
As shown in the output, the tracked balance of the Treasury is 0 because deposit() is not used.
The actual balance of the Treasury > 0, but withdraw() reverts.
Impact
FeeCollector emergency withdraw funds are permanently locked in the Treasury contract
Tools Used
Manual Review
Recommendations
Modify FeeCollector::emergencyWithdraw() and FeeCollector::_processDistributions() to call Treasury::deposit() instead of safeTransfer().
