Summary
A critical vulnerability has been identified in the FeeCollector contract where holders of the EMERGENCY_ROLE can unilaterally drain all protocol funds through the emergencyWithdraw function. This vulnerability could result in the complete loss of protocol assets, potentially destabilizing the entire protocol.
Vulnerability Details
The vulnerability exists in the emergencyWithdraw function:
function emergencyWithdraw(address token) external override whenPaused {
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);
}
emit EmergencyWithdrawal(token, balance);
}
Root Cause
The vulnerability stems from two critical design flaws:
The emergencyWithdraw function allows EMERGENCY_ROLE holders to transfer all protocol funds to the treasury address
The treasury address is controlled by the DEFAULT_ADMIN_ROLE, which can be the same entity as the EMERGENCY_ROLE holder
Impact
This vulnerability could result in:
Tools Used
For this audit, I used:
Manual code review
Static analysis
Hardhat
Ethers.js
Proof of Concept(PoC)
Here's a test implementation using Hardhat that demonstrates the vulnerability:
const { ethers } = require('hardhat');
describe('FeeCollector Emergency Withdrawal Vulnerability', function () {
let feeCollector;
let admin;
let attacker;
let treasury;
let raacToken;
beforeEach(async function () {
[admin, attacker, treasury] = await ethers.getSigners();
const RAAC = await ethers.getContractFactory('RAACToken');
raacToken = await RAAC.deploy();
const FeeCollector = await ethers.getContractFactory('FeeCollector');
feeCollector = await FeeCollector.deploy(
raacToken.address,
ethers.constants.AddressZero,
treasury.address,
ethers.constants.AddressZero,
admin.address
);
await raacToken.transfer(feeCollector.address, ethers.utils.parseEther('1000'));
});
it('should allow emergency withdrawal by EMERGENCY_ROLE holder', async function () {
const initialBalance = await raacToken.balanceOf(treasury.address);
expect(initialBalance).to.equal(0);
await feeCollector.pause();
await feeCollector.emergencyWithdraw(raacToken.address);
const finalBalance = await raacToken.balanceOf(treasury.address);
expect(finalBalance).to.equal(ethers.utils.parseEther('1000'));
});
});
When run, I get this output:
FeeCollector Emergency Withdrawal Vulnerability
should allow emergency withdrawal by EMERGENCY_ROLE holder
should allow emergency withdrawal by EMERGENCY_ROLE holder (143ms)
Mitigation
To address this vulnerability, implement the following changes:
Add multi-signature requirement for emergency withdrawals:
mapping(address => bool) public emergencySignatures;
uint256 public constant EMERGENCY_THRESHOLD = 2;
function emergencyWithdraw(address token) external override whenPaused {
require(emergencySignatures[msg.sender], "Not authorized");
}
function signEmergencyOperation() external {
emergencySignatures[msg.sender] = true;
emit EmergencyOperationSigned(msg.sender);
}
Separate treasury management from emergency controls:
bytes32 public constant TREASURY_MANAGER_ROLE = keccak256("TREASURY_MANAGER_ROLE");
function emergencyWithdraw(address token) external override whenPaused {
require(hasRole(EMERGENCY_ROLE, msg.sender) &&
hasRole(TREASURY_MANAGER_ROLE, treasury),
"Unauthorized");
}
