code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-11-stakehouse/blob/4b6828e9c807f2f7c569e6d721ca1289f7cf7112/contracts/liquid-staking/SyndicateRewardsProcessor.sol#L51-L73

Vulnerability details

Impact

The totalClaimed amount may have the wrong due balance . totalClaimed receive the due amount even if any failure in call functions.

Proof of Concept

   function _distributeETHRewardsToUserForToken(
    address _user,
    address _token,
    uint256 _balance,
    address _recipient
) internal {
    require(_recipient != address(0), "Zero address");
    uint256 balance = _balance;
    if (balance > 0) {
        // Calculate how much ETH rewards the address is owed / due 
        uint256 due = ((accumulatedETHPerLPShare * balance) / PRECISION) - claimed[_user][_token];
        if (due > 0) {
            claimed[_user][_token] = due;

            totalClaimed += due;

            (bool success, ) = _recipient.call{value: due}("");
            require(success, "Failed to transfer");

            emit ETHDistributed(_user, _recipient, due);
        }
    }
}

There is no reverting mechanism followed if any failure is in call function.

The status only checked after due amount is added to totalClaimed . If call operation fails its only through the error .

Manual Audit

Recommended Mitigation Steps

Need to decrease the due amount from totalClaimed  if any failure occurs in call function.

dmvt commented 1 year ago

Incorrect. If the function call rolls back due to the failed require statement, the change to totalClaimed will also be rolled back. This type of issue is why it is highly recommended that wardens write tests to prove their reports.

c4-judge commented 1 year ago

dmvt marked the issue as unsatisfactory: Invalid

code-423n4 / 2022-11-stakehouse-findings