Core Contracts
Regnum Aurum Acquisition Corp
HardhatReal World AssetsNFT
77,280 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Tokens sent directly to the treasury are lost forever due to the internal balance tracking

avoloder

Summary

Any tokens sent directly to the treasury (e.g., via transfer, safeTransfer) will be lost forever, as Treasury.sol tracks balances internally and does not provide an option to withdraw tokens that are not sent through its deposit function

Vulnerability Details

Treasury.sol manages the protocol's treasury funds with role-based access control. It contains a mapping(address => uint256) _balances where the balances of certain tokens are tracked. Additionally, it has functions to deposit and withdraw tokens, which are defined as follows:

function deposit(address token, uint256 amount) external override nonReentrant {
if (token == address(0)) revert InvalidAddress();
if (amount == 0) revert InvalidAmount();
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 (token == address(0)) revert InvalidAddress();
if (recipient == address(0)) revert InvalidRecipient();
if (_balances[token] < amount) revert InsufficientBalance();
_balances[token] -= amount;
_totalValue -= amount;
IERC20(token).transfer(recipient, amount);
emit Withdrawn(token, amount, recipient);
}

As can be seen, with every deposit — that is, with every withdrawal — the _balances mapping is updated. The problem arises if ERC20 tokens are sent to the contract via ERC20 functions like transfer or safeTransfer (e.g., in FeeCollector.sol), because in that case, the _balances mapping will not be updated. Since there is a subsequent check inside the withdraw function (if (_balances[token] < amount) revert InsufficientBalance();), these tokens will not be withdrawable, meaning they are lost forever.

POC

Add this test to the Treasury.test.js

describe("Direct transfer", () => {
it("should revert if the withdrawal is initiated after direct transfer", async () => {
await token.connect(user1).approve(owner.address, 95);
​
// transfer tokens via `transferFrom` directly
await token.transferFrom(user1.address, treasury.target, 95);
​
// confirm the balance of `Treasury` using ERC20 `balanceOf` function
const treasuryBalance = await token.balanceOf(treasury.target);
expect(treasuryBalance).to.equal(95);
​
// check the balance of `Treasury` using its own method `getBalance`
expect(await treasury.getBalance(token.getAddress())).to.equal(0);
​
// try to withdraw tokens from the contract
await expect(treasury.connect(manager).withdraw(token.getAddress(), 95, manager.address)).to.be.revertedWithCustomError(treasury, "InsufficientBalance()");
​
});
});

Impact

High, since the tokens will be lost forever and there are multiple places in the system where tokens are sent directly via ERC20 functions

Tools Used

Manual review

Recommendations

There is no need to track the balances of each token in a separate mapping, since ERC20 tokens have a built-in balanceOf() function that can be called to check the balance.

If you wanted to avoid donations and direct transfers, for whatever reason, and strictly want to use internal balance tracking, than you need to adjust the logic in all other places to use Treasury::deposit() function instead of ERC20::transfer() and ERC20::safeTransfer() whenever you are sending tokens to the treasury.

Updates

Lead Judging Commences

inallhonesty Lead Judge 4 months ago
Submission Judgement Published
Validated
Assigned finding tags:

FeeCollector::_processDistributions and emergencyWithdraw directly transfer funds to Treasury where they get permanently stuck

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.