Submission Details
Severity:
Overwriting Previous Allocations in allocateFunds May Lead to Loss of Cumulative Allocation Data
Summary
The `allocateFunds()` function in `Treasury.sol` overwrites any existing allocation from the caller to a recipient rather than accumulating new allocations. This behavior can inadvertently discard previously allocated funds when multiple allocations are made, leading to inaccurate fund distribution and management.
Vulnerability Details
In its current implementation, the allocateFunds() function directly assigns the provided amount to the allocation mapping without considering any prior allocation:
```solidity
function allocateFunds(address recipient, uint256 amount) external override onlyRole(ALLOCATOR_ROLE) {
if (recipient == address(0)) revert InvalidRecipient();
if (amount == 0) revert InvalidAmount();
_allocations[msg.sender][recipient] = amount;
emit FundsAllocated(recipient, amount);
}
```
This means that if an allocator makes multiple allocations to the same recipient, each new allocation will replace the previous one instead of being added together. For example, if the allocator first assigns 1,000 tokens and later assigns an additional 500 tokens, the final recorded allocation will be 500 tokens—not the expected cumulative total of 1,500 tokens.
### Proof of Concept
The following unit test demonstrates the issue:
```solidity
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.19;
import "forge-std/Test.sol";
import "../../../contracts/core/collectors/Treasury.sol";
import "lib/openzeppelin-contracts/contracts/mocks/token/ERC20Mock.sol";
contract MockERC20 is ERC20 {
constructor(string memory name, string memory symbol) ERC20(name, symbol) {
_mint(msg.sender, 100_000 * 10 ** 18); // Mint 100k tokens to deployer
}
}
contract TreasuryTest is Test {
Treasury treasury;
MockERC20 token;
address admin = address(0x1);
address manager = address(0x2);
address allocator = address(0x3);
address user = address(0x7);
address userA = address(0x4);
address userB = address(0x6);
address recipient = address(0x5);
// Role hashes
bytes32 constant MANAGER_ROLE = keccak256("MANAGER_ROLE");
bytes32 constant ALLOCATOR_ROLE = keccak256("ALLOCATOR_ROLE");
function setUp() public {
// Deploy mock token
token = new MockERC20("Mock Token", "MTK");
// Deploy Treasury contract with admin
vm.prank(admin);
treasury = new Treasury(admin);
// Grant roles
vm.startPrank(admin);
treasury.grantRole(MANAGER_ROLE, manager);
treasury.grantRole(ALLOCATOR_ROLE, allocator);
vm.stopPrank();
// Transfer some tokens to user for testing deposits
token.transfer(userA, 10_000 * 10 ** 18);
token.transfer(userB, 10_000 * 10 ** 18);
}
function testAllocateFundsOverwritesPreviousAllocation() public {
// Define allocation amounts
uint256 initialAllocation = 1_000 * 10**18; // First allocation
uint256 newAllocation = 500 * 10**18; // Second allocation
// Perform first allocation as allocator
vm.prank(allocator);
treasury.allocateFunds(recipient, initialAllocation);
// Verify initial allocation
uint256 initialRecorded = treasury.getAllocation(allocator, recipient);
assertEq(initialRecorded, initialAllocation, "Initial allocation not recorded correctly");
console.log("Initial Allocation:", initialRecorded / 10**18, "units");
// Perform second allocation to the same recipient
vm.prank(allocator);
treasury.allocateFunds(recipient, newAllocation);
// Verify new allocation overwrites the previous one
uint256 finalRecorded = treasury.getAllocation(allocator, recipient);
assertEq(finalRecorded, newAllocation, "Second allocation should overwrite the first");
assertNotEq(finalRecorded, initialAllocation + newAllocation, "Allocation should not accumulate");
console.log("Final Allocation after overwrite:", finalRecorded / 10**18, "units");
// Highlight the issue: Expected accumulation would be 1500, but we get 500
console.log("Expected if accumulated:", (initialAllocation + newAllocation) / 10**18, "units");
console.log("Actual (overwritten):", finalRecorded / 10**18, "units");
}
}
```
Test Output:
```yaml
Logs:
Initial Allocation: 1000 units
Final Allocation after overwrite: 500 units
Expected if accumulated: 1500 units
Actual (overwritten): 500 units
```
Impact
Loss of Cumulative Allocations: Subsequent allocations overwrite previous ones, which can lead to unintentional reduction of the total funds allocated.
Operational Errors: Allocators may assume that allocations add up over time, causing mismanagement of funds if their expectations are not met.
Inaccurate Record Keeping: The absence of cumulative tracking leads to discrepancies in fund distribution records, undermining trust and transparency in the system.
Tools Used
manual review and foundry
Recommendations
To prevent overwriting previous allocations, modify the function so that it accumulates new allocations with any existing ones. For example:
```solidity
function allocateFunds(address recipient, uint256 amount) external override onlyRole(ALLOCATOR_ROLE) {
if (recipient == address(0)) revert InvalidRecipient();
if (amount == 0) revert InvalidAmount();
// Accumulate new allocation instead of overwriting
_allocations[msg.sender][recipient] += amount;
emit FundsAllocated(recipient, _allocations[msg.sender][recipient]);
}
```
This adjustment ensures that multiple allocations from the same allocator to a recipient will add up, preserving the total allocation amount and ensuring accurate tracking of allocated funds.
Updates
Lead Judging Commences
inallhonesty 6 months ago
Submission Judgement Published Assigned finding tags:
Treasury::allocateFunds should increase or decrease funds to avoid recipient frontrunning and double spending
Prize pool breakdown
Total prize
77,280 USDC
nSLOC
3,86420
USDC / LOC
74,000
3,280
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.