The DevFund contract acts as a donation and dao treasury that can fund approved developers based on agreed weighting given to each developer. However, due to rounding errors, unfavorable conditions can cause entire donation amounts to be transferred to the owner account instead of the approved developers.
The following snippet shows how rewards are split, allocating excess amounts to the owner:
uint256 amountPerWeight = msg.value / totalDevWeight; // @audit if msg.value < totalDevWeight, then amountPerWeight will be 0
uint256 remaining = msg.value - (amountPerWeight * totalDevWeight); // @audit if msg.value < totalDevWeight, then remaining will be msg.value
totalRewardDebt += amountPerWeight;
if (remaining > 0) {
(bool success, ) = payable(owner()).call{ value: remaining }('');
require(success, 'Failed to send Ether to owner');
}
If msg.value < totalDevWeight the donation sent to the receive() function will result in an amountPerWeight == 0. This results in remaining == msg.value being sent to the owner.
This is unintended behavior and it can't be expected that every account donating should donate an amount of at least totalDevWeight or risk there funds being transferred elsewhere.
Proof of Concept
A snapshot of the test that reproduces this issue is located below. For full test suite and setup please go to the following repository.
The command to run the test is forge test --match-test testFailOwnerReceivesRemainingWhenTotalDevWeightLessThanFunding
function testFailOwnerReceivesRemainingWhenTotalDevWeightLessThanFunding() public {
uint256 devWeight = 100 ether;
uint256 fundingAmount = 10 ether;
// Add a dev with weight less than funding amount
devFund.addDev(dev1, devWeight);
// Record initial balances
uint256 initialOwnerBalance = owner.balance;
uint256 initialContractBalance = address(devFund).balance;
// Fund the contract
payable(address(devFund)).call{value: fundingAmount}("");
// Check that the contract balance is equal to the funding amount
assertLt(owner.balance, initialOwnerBalance + fundingAmount, "Owner should not receive the full funding amount");
assertTrue(address(devFund).balance != 0, "DevFund balance should not be 0");
}
Tools Used
Manual Review, Forge/Foundry
Recommended Mitigation Steps
Several options should be considered;
Reverting and refunding if the amount is not sufficient to match the totalDevWeight > msg.value (use this method if the expectation is that users are only allowed to donate successfully if their donation is at least totalDevWeight)
If this isn't the case remove the excess transfer functionality to the owner. If additional funds are left in the contract, provide a way to clean those funds up after the funds have been distributed to the developers.
Lines of code
https://github.com/code-423n4/2024-07-traitforge/blob/279b2887e3d38bc219a05d332cbcb0655b2dc644/contracts/DevFund/DevFund.sol#L16-L18
Vulnerability details
Impact
The
DevFund
contract acts as a donation and dao treasury that can fund approved developers based on agreed weighting given to each developer. However, due to rounding errors, unfavorable conditions can cause entire donation amounts to be transferred to theowner
account instead of the approved developers.The following snippet shows how rewards are split, allocating excess amounts to the owner:
If
msg.value < totalDevWeight
the donation sent to thereceive()
function will result in anamountPerWeight == 0
. This results inremaining == msg.value
being sent to the owner.This is unintended behavior and it can't be expected that every account donating should donate an amount of at least
totalDevWeight
or risk there funds being transferred elsewhere.Proof of Concept
A snapshot of the test that reproduces this issue is located below. For full test suite and setup please go to the following repository.
The command to run the test is
forge test --match-test testFailOwnerReceivesRemainingWhenTotalDevWeightLessThanFunding
Tools Used
Manual Review, Forge/Foundry
Recommended Mitigation Steps
Several options should be considered;
totalDevWeight > msg.value
(use this method if the expectation is that users are only allowed to donate successfully if their donation is at leasttotalDevWeight
)Assessed type
Rug-Pull