# Rounding Errors Cause DevFund Donations To Be Stolen By Owner

[c4-bot-2]

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 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;

1. 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)
2. 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.

Assessed type

Rug-Pull

[kamensec]

This vulnerability was marked as invalid with comments 'msg.value is ETH with 18 decimals'. The actual vulnerability is not reliant on any specific msg.value, more so that msg.value > _forgerListingInfo.fee and msg.value < totalDevWeight totalDevWeight is something accrued over time as more devs are added to the fund. If the totalDevWeight is made too large, small value donations will be sent to the owner unintentionally.

[koolexcrypto]

If the totalDevWeight is made too large, small value donations will be sent to the owner unintentionally

A general statement and theoretical.

Lack of specific numbers can not help for assessing the severity. Furthermore, totalDevWeight is controlled by the owner.

Given above, this would be a QA