Description
After reviewing FeeCollector.sol, I can confirm the report is valid but requires more detailed technical analysis. The contract has critical centralization risks in its treasury management system.
Vulnerable Code
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);
}
}
function setTreasury(address newTreasury) external override {
if (!hasRole(DEFAULT_ADMIN_ROLE, msg.sender)) revert UnauthorizedCaller();
if (newTreasury == address(0)) revert InvalidAddress();
pendingTreasury = PendingUpdate({
newAddress: newTreasury,
effectiveTime: block.timestamp + TREASURY_UPDATE_DELAY
});
emit TreasuryUpdated(newTreasury);
}
Impact Analysis
-
Emergency Control Concentration
Single EMERGENCY_ROLE can pause contract
Same role can then withdraw all tokens
No multi-signature requirement
-
Treasury Update Risks
1. Admin calls setTreasury(attackerAddress)
2. Wait TREASURY_UPDATE_DELAY (1 day)
3. Call applyTreasuryUpdate()
4. Pause contract
5. Call emergencyWithdraw() to malicious treasury
-
Insufficient Access Controls
DEFAULT_ADMIN_ROLE can grant EMERGENCY_ROLE
No separation of concerns between roles
Single account can accumulate all privileges
Proof of Concept
contract TreasuryAttackTest {
FeeCollector feeCollector;
address attacker;
function testTreasuryDrain() public {
vm.startPrank(admin);
feeCollector.grantRole(EMERGENCY_ROLE, attacker);
feeCollector.setTreasury(attacker);
vm.warp(block.timestamp + 1 days);
feeCollector.applyTreasuryUpdate();
feeCollector.pause();
feeCollector.emergencyWithdraw(address(raacToken));
assertEq(raacToken.balanceOf(attacker), initialBalance);
}
}
Recommended Mitigation
contract FeeCollector {
uint256 public constant MIN_TIMELOCK_DELAY = 2 days;
uint256 public constant EMERGENCY_THRESHOLD = 3;
mapping(bytes32 => uint256) public proposalApprovals;
mapping(address => bool) public emergencyCouncil;
modifier requiresMultiApproval(bytes32 proposalId) {
require(proposalApprovals[proposalId] >= EMERGENCY_THRESHOLD,
"Insufficient approvals");
_;
}
function setTreasury(address newTreasury) external {
bytes32 proposalId = keccak256(abi.encodePacked(
"setTreasury",
newTreasury,
block.timestamp
));
require(emergencyCouncil[msg.sender], "Not council member");
proposalApprovals[proposalId]++;
if(proposalApprovals[proposalId] >= EMERGENCY_THRESHOLD) {
pendingTreasury = PendingUpdate({
newAddress: newTreasury,
effectiveTime: block.timestamp + MIN_TIMELOCK_DELAY
});
emit TreasuryUpdateProposed(newTreasury);
}
}
function emergencyWithdraw(address token) external
whenPaused
requiresMultiApproval(keccak256(abi.encodePacked(
"emergencyWithdraw",
token,
block.timestamp
)))
{
}
}
Additional Recommendations
-
Implement role separation:
Treasury management
Emergency controls
Fee distribution
-
Add timelock delays:
uint256 public constant EMERGENCY_DELAY = 24 hours;
uint256 public constant TREASURY_DELAY = 48 hours;
-
Require multiple signatures:
-
Enhanced event logging:
event EmergencyActionProposed(
bytes32 indexed proposalId,
address indexed proposer,
uint256 timestamp
);
