Treasury Vesting - BlockDAG

The allocateTokens function in the TreasuryVesting contract contains a vulnerability that allows operators to allocate more tokens than the category's total amount limit. This occurs because the check that's meant to enforce the category limit uses vesting.released, which is never updated during token allocation or release, making the limit check ineffective.

function allocateTokens(address user, bytes32 category, uint256 amount) external {
    // ...
    require(vesting.released + amount <= vesting.totalAmount, "Exceeds category limit");
    // vesting.released is never updated, always remains 0
    // ...
}

The vulnerability allows unlimited token allocations within a category, bypassing the intended total amount limit. This could lead to:

• More tokens being allocated than intended by the protocol

• Potential insolvency if more tokens are promised than available

• Breaking of tokenomics and vesting schedules

• Loss of funds if the contract doesn't have enough tokens to cover all allocations

Here's a test function proving that token allocations can exceed total amount:

function test_ExceedCategoryLimit() public {
    // Setup category with 1000e18 total amount limit
    uint256[] memory releaseSteps = new uint256[](1);
    uint256[] memory timeSteps = new uint256[](1);
    releaseSteps[0] = 10000;
    timeSteps[0] = 0;

    vm.startPrank(admin);
    bytes32 operationId = keccak256(
        abi.encode(
            treasuryVesting.OPERATION_ADD_CATEGORY(),
            treasuryVesting.EARLY_BIRD_CATEGORY(),
            block.timestamp,
            0,
            1000e18,
            releaseSteps,
            timeSteps
        )
    );

    treasuryVesting.scheduleAddCategory(
        treasuryVesting.EARLY_BIRD_CATEGORY(),
        block.timestamp,
        0,
        1000e18,
        releaseSteps,
        timeSteps
    );

    vm.warp(block.timestamp + treasuryVesting.TIMELOCK_DURATION_ADD_CATEGORY());
    treasuryVesting.executeAddCategory(operationId);
    vm.stopPrank();

    vm.startPrank(operator);
    // Allocate maximum amount to first user
    treasuryVesting.allocateTokens(user1, treasuryVesting.EARLY_BIRD_CATEGORY(), 1000e18);
    
    // Should fail but succeeds: allocate same amount to second user
    treasuryVesting.allocateTokens(user2, treasuryVesting.EARLY_BIRD_CATEGORY(), 1000e18);
    vm.stopPrank();

    // Verify total allocated amount exceeds limit
    uint256 totalAllocated = treasuryVesting.getAllocation(user1, treasuryVesting.EARLY_BIRD_CATEGORY()) +
                            treasuryVesting.getAllocation(user2, treasuryVesting.EARLY_BIRD_CATEGORY());
    assertEq(totalAllocated, 2000e18); // Double the intended limit
}

Replace the current check with a tracking mechanism for total allocations.

SOLVED: The BlockDAG team solved this issue as follows:

• Adds proper tracking of total allocations via totalAllocated

• Changes the limit check to use total allocations instead of released amounts

• Updates the running total when new allocations are made

• Maintains the same check in both single and batch allocation functions

The releaseTokens and batchRelease functions lack access control, allowing anyone to release tokens:

function releaseTokens(
        address user,
        bytes32 category
    ) external nonReentrant whenNotPaused returns (uint256)

function batchRelease(
        bytes32 category,
        address[] calldata users
    ) external nonReentrant whenNotPaused

This implementation does not follow the intention as stated in the provided documentation:

3.3 Admin-Only Release

• The admin triggers batch release for each category after verifying the correct vesting stage has arrived.

Add appropriate access control to both release functions.

SOLVED: The BlockDAG team solved this issue as follows:

• Adds the onlyRole(ADMIN_ROLE) modifier to both functions

• Ensures only authorized admins can trigger token releases

• Maintains existing nonReentrant and pause checks

• Aligns with the requirement for admin-only releases

The TreasuryVesting contract implements two different patterns for handling operations. While standard operations (like adding categories) use a structured TimelockOperation system with proper expiry and cleanup mechanisms, emergency operations (pause/unpause) use a simplified direct hashing approach.

// Emergency operations use direct hashing
bytes32 pauseOperationId = keccak256("EMERGENCY_PAUSE");

// While standard operations use TimelockOperation struct
struct TimelockOperation {
    bytes32 operationId;
    uint256 executeTime;
    uint256 expiryTime;
    bool executed;
    bytes encodedParams;
}

This inconsistency prevents the cleanup function from working properly for emergency operations, as they don't exist in the timelockOperations mapping.

Impact:

• Stale pause/unpause approvals remain in the system

• The approval state doesn't get reset after execution

• This could lead to confusion and potential security issues if old approvals are reused

• Standardize the operation handling:

  • Use the same timelock and operation ID pattern for all operations including pause/unpause

  • Include pause/unpause in the standard operation types

• Reset approval states after execution:

  • Clear approvals after successful pause/unpause

  • Implement proper cleanup for all operation types

• Add proper expiry mechanism for pause/unpause operations

SOLVED: The suggested mitigation was implemented by the BlockDAG team - Use the same timelock and operation ID pattern for all operations including pause/unpause

In TreasuryVesting::approveOperation() function, when a user with both ADMIN_ROLE and OPERATOR_ROLE approves an operation, both approval counters are incremented, effectively giving them two votes from a single address.

function approveOperation(bytes32 operationId) external {
    require(hasRole(ADMIN_ROLE, msg.sender) || hasRole(OPERATOR_ROLE, msg.sender), "Not authorized");

    MultiSigApproval storage approval = multiSigApprovals[operationId];
    require(!approval.hasApproved[msg.sender], "Already approved");
    require(!approval.executed, "Operation already executed");

    approval.hasApproved[msg.sender] = true;

    // Issue: Both counters increment if user has both roles
    if (hasRole(ADMIN_ROLE, msg.sender)) {
        approval.adminApprovalCount++;
    }
    if (hasRole(OPERATOR_ROLE, msg.sender)) {
        approval.operatorApprovalCount++;
    }

    emit MultiSigApprovalSubmitted(
        operationId, msg.sender, approval.adminApprovalCount, approval.operatorApprovalCount
    );
}

This undermines the security of the multi-signature system by allowing users with multiple roles to have disproportionate voting power.

Add the following to the foundry test file:

   // Test multi-sig approval tracking
    function test_MultiSigApprovalTracking() public {
        uint256[] memory releaseSteps = new uint256[](1);
        uint256[] memory timeSteps = new uint256[](1);
        releaseSteps[0] = 10000;
        timeSteps[0] = 0;

        vm.startPrank(admin);
        bytes32 operationId = keccak256(
            abi.encode(
                treasuryVesting.OPERATION_ADD_CATEGORY(),
                treasuryVesting.EARLY_BIRD_CATEGORY(),
                block.timestamp,
                100000e18,
                releaseSteps,
                timeSteps
            )
        );

        treasuryVesting.scheduleAddCategory(
            treasuryVesting.EARLY_BIRD_CATEGORY(), block.timestamp, 100000e18, releaseSteps, timeSteps
        );

        // Track admin approvals
        treasuryVesting.approveOperation(operationId);
        (uint256 adminCount, uint256 operatorCount,,) = treasuryVesting.multiSigApprovals(operationId);
        assertEq(adminCount, 1);
        assertEq(operatorCount, 0);
        vm.stopPrank();

        // Track operator approvals
        vm.prank(operator);
        treasuryVesting.approveOperation(operationId);
        (adminCount, operatorCount,,) = treasuryVesting.multiSigApprovals(operationId);
        assertEq(adminCount, 1);
        assertEq(operatorCount, 1);
    }

Output:

The assertions fail, proving the failing integrity of approval counts.

Modify the approval logic to use mutually exclusive conditions, ensuring a user can only increment one counter even if they have multiple roles:

function approveOperation(bytes32 operationId) external {
    require(hasRole(ADMIN_ROLE, msg.sender) || hasRole(OPERATOR_ROLE, msg.sender), "Not authorized");

    MultiSigApproval storage approval = multiSigApprovals[operationId];
    require(!approval.hasApproved[msg.sender], "Already approved");
    require(!approval.executed, "Operation already executed");

    approval.hasApproved[msg.sender] = true;

    // Fix: Mutually exclusive conditions prevent double counting
    if (hasRole(ADMIN_ROLE, msg.sender)) {
        approval.adminApprovalCount++;
    } else if (hasRole(OPERATOR_ROLE, msg.sender)) {
        approval.operatorApprovalCount++;
    }

    emit MultiSigApprovalSubmitted(
        operationId, msg.sender, approval.adminApprovalCount, approval.operatorApprovalCount
    );
}

SOLVED: The suggested mitigation was implemented by BlockDAG team.

The current implementation doesn't distinguish between operator and admin approvals in the approveOperation function. Both operators and admins contribute to the same approval count, which could lead to security risks.

function approveOperation(bytes32 operationId) external {
    require(hasRole(ADMIN_ROLE, msg.sender) || hasRole(OPERATOR_ROLE, msg.sender), "Not authorized");

    MultiSigApproval storage approval = multiSigApprovals[operationId];
    require(!approval.hasApproved[msg.sender], "Already approved");
    require(!approval.executed, "Operation already executed");

    approval.hasApproved[msg.sender] = true;
    approval.approvalCount++;  // Both operator and admin approvals increment the same counter
}

Impact:

• Operators could potentially contribute to admin-level operations

• The system doesn't properly enforce the separation between REQUIRED_ADMIN_SIGNATURES (3) and REQUIRED_OPERATOR_SIGNATURES (2)

• This could lead to unauthorized execution of sensitive operations

Separate the approval tracking for operators and admins:

• Add separate counters for admin and operator approvals

• Validate the appropriate threshold based on the operation type

• Implement proper role-based approval counting

SOLVED: Separate counters for admin and operator approvals were implemented by the BlockDAG team.

Timelock operations don't have an expiration time, allowing them to be executed indefinitely after the timelock period:

    struct TimelockOperation {
        bytes32 operationId;     // Unique identifier for the operation
        uint256 executeTime;     // When the operation can be executed
        bool executed;           // Whether operation has been completed
        bytes encodedParams;     // Encoded function parameters
    }

This means old operations could be executed long after they become irrelevant.

Add expiration time to TimelockOperation struct:

struct TimelockOperation {
    bytes32 operationId;
    uint256 executeTime;
    uint256 expiryTime;
    bool executed;
    bytes encodedParams;
}

SOLVED: Added expiration time to TimelockOperation struct.

The executeAddCategory function in the TreasuryVesting contract allows the creation of arbitrary categories without validating if they match one of the predefined category types (EARLY_BIRD_CATEGORY, PRESALE_CATEGORY, or TEAM_CATEGORY). While the function includes specific validation logic for known categories, it doesn't prevent the creation of undefined categories:

function executeAddCategory(bytes32 operationId) external onlyRole(ADMIN_ROLE) {
    // ... decode parameters ...
    
    // Category-specific validations
    if (category == EARLY_BIRD_CATEGORY) {
        // Early Bird validations
    } 
    else if (category == PRESALE_CATEGORY) {
        // Presale validations
    } 
    else if (category == TEAM_CATEGORY) {
        // Team validations
    }
    // No else clause to prevent undefined categories

Impact:

• Allows creation of non-standard vesting categories

• Could lead to confusion in token distribution

• Inconsistent vesting rules across the protocol

Add explicit category validation at the start of the function:

function executeAddCategory(bytes32 operationId) external onlyRole(ADMIN_ROLE) {
    TimelockOperation storage operation = timelockOperations[operationId];
    require(operation.operationId != bytes32(0), "Operation doesn't exist");
    require(!operation.executed, "Already executed");
    require(block.timestamp >= operation.executeTime, "Timelock not expired");

    operation.executed = true;

    // Decode operation parameters
    (
        bytes32 category,
        uint256 start,
        uint256 duration,
        uint256 totalAmount,
        uint256[] memory releaseSteps,
        uint256[] memory timeSteps
    ) = abi.decode(
        operation.encodedParams,
        (bytes32, uint256, uint256, uint256, uint256[], uint256[])
    );

    // Add explicit category validation
    require(
        category == EARLY_BIRD_CATEGORY ||
        category == PRESALE_CATEGORY ||
        category == TEAM_CATEGORY,
        "Invalid category type"
    );

    // Rest of the function...
}

SOLVED: The BlockDAG team solved this issue as follows:

• Added explicit validation of category types at the start

• Maintains all existing category-specific validations

• Ensures only predefined categories can be created

• Keeps the error message clear and descriptive

The TreasuryVesting contract is designed to be used with the proxy pattern, as evidenced by its inheritance of Initializable and use of the initialize() function. However, the contract does not disable the initializer in its constructor, leaving it vulnerable to potential initialization attacks:

contract TreasuryVesting is 
    Initializable,
    AccessControlUpgradeable,
    ReentrancyGuardUpgradeable,
    PausableUpgradeable
{
    
    function initialize(
        address admin,
        address _bdagToken
    ) public initializer {
        // ... initialization logic ...
    }
}

Add a constructor that disables initializers:

contract TreasuryVesting is 
    Initializable,
    AccessControlUpgradeable,
    ReentrancyGuardUpgradeable,
    PausableUpgradeable
{
    /// @custom:oz-upgrades-unsafe-allow constructor
    constructor() {
        _disableInitializers();
    }
    
    function initialize(
        address admin,
        address _bdagToken
    ) public initializer {
        // ... rest of initialization logic ...
    }
}

This ensures that the implementation contract cannot be initialized directly, following the proper proxy pattern implementation.

SOLVED:

• Adds the constructor with _disableInitializers()

• Includes the OpenZeppelin annotation for upgrades safety

• Maintains all existing initialization logic

• Prevents potential reinitialization attacks

• Follows best practices for upgradeable contracts

The getReleasableAmount function in the TreasuryVesting contract appears to lack validation for the duration field. However, this is not a security concern as the token release schedule is effectively controlled by the releaseSteps and timeSteps arrays, which are properly validated during category creation:

function getReleasableAmount(address user, bytes32 category) public view returns (uint256) {
    CategoryVesting storage vesting = categoryVestings[category];
    if (block.timestamp < vesting.start) return 0;
    
    uint256 allocation = userAllocations[user][category];
    uint256 totalReleased = userReleased[user][category];
    uint256 releasable = 0;
    
    // Release schedule is controlled by timeSteps
    for (uint256 i = 0; i < vesting.timeSteps.length; i++) {
        if (block.timestamp >= vesting.start + vesting.timeSteps[i]) {
            releasable += (allocation * vesting.releaseSteps[i]) / 10000;
        }
    }
    
    return releasable > totalReleased ? releasable - totalReleased : 0;
}

Impact:

• Code readability and maintainability could be improved

• duration field in CategoryVesting struct is effectively redundant

• Minor gas cost for storing unused duration value

Consider removing the redundant duration field or using it for validation.

SOLVED: Removed the redundant duration field.

TreasuryVesting contract relies heavily on admin and operator roles for critical functions. Therefore it creates single points of failure if admin/operator keys are compromised.

It is recommended to implement multi-signature requirements for admin/operator actions.

SOLVED: The BlockDAG team solved this issue as follows:

• Requires multiple signatures for admin and operator actions

• Tracks approvals per operation

• Clears approvals after execution

• Emits events for transparency

• Maintains existing role-based access control

The following constants in the TreasuryVesting contract are not used:

bytes32 public constant OPERATION_UPDATE_SCHEDULE = keccak256("UPDATE_SCHEDULE");
bytes32 public constant OPERATION_PAUSE = keccak256("OPERATION_PAUSE");
bytes32 public constant OPERATION_UNPAUSE = keccak256("OPERATION_UNPAUSE");

It is recommended to remove them for code clarity and maintainability.

SOLVED: The suggested mitigation was implemented.