code-423n4 / 2024-05-olas-findings

12 stars 3 forks source link

if amountETHFromServices< accountRewards and topup is non zero, account doesn't receive rewards but it gets deducted from rewards balance #77

Closed howlbot-integration[bot] closed 2 months ago

howlbot-integration[bot] commented 2 months ago

Lines of code

https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/Dispenser.sol#L809-L832 https://github.com/code-423n4/2024-05-olas/blob/main/tokenomics/contracts/Treasury.sol#L402-L419

Vulnerability details

Impact

When claiming owner incentives in the Dispenser contract, if amountETHFromServices< accountRewards, the user's reward balance will get updated, but he won't receive the reward

Proof of Concept

Dispenser#claimOwnerIncentives can be used to claim incentives for the owner of components/agents:

    function claimOwnerIncentives(
        uint256[] memory unitTypes,
        uint256[] memory unitIds
    ) external returns (uint256 reward, uint256 topUp) {
        ...
        (reward, topUp) = ITokenomics(tokenomics).accountOwnerIncentives(
            msg.sender,
            unitTypes,
            unitIds
        );

        bool success;
        if ((reward + topUp) > 0) {
            ...
            success = ITreasury(treasury).withdrawToAccount(
                msg.sender,
                reward,
                topUp
            );
            ...
        }

        if (!success) {
            revert ClaimIncentivesFailed(msg.sender, reward, topUp);
        }
        ...
    }

Here is Treasury#withdrawToAccount:

    function withdrawToAccount(
        address account,
        uint256 accountRewards,
        uint256 accountTopUps
    ) external returns (bool success) {
        ...
        if (accountRewards > 0 && amountETHFromServices >= accountRewards) {
            amountETHFromServices -= accountRewards;
            ETHFromServices = uint96(amountETHFromServices);
            emit Withdraw(ETH_TOKEN_ADDRESS, account, accountRewards);
            (success, ) = account.call{value: accountRewards}("");
            if (!success) {
                revert TransferFailed(
                    address(0),
                    address(this),
                    account,
                    accountRewards
                );
            }
        }
        if (accountTopUps > 0) {
            // Tokenomics has already accounted for the account's top-up amount,
            // thus the the mint does not break the inflation schedule
            IOLAS(olas).mint(account, accountTopUps);
            success = true;
            emit Withdraw(olas, account, accountTopUps);
        }
    }

We can see that only if accountRewards>0 && amountETHFromServices>=accountRewards, the if block gets executed. If amountETHFromServices < accountRewards, the if block, which contains logic for sending the rewards won't be executed.

Still looking at the withdrawAccount function, if the topup>0, olas gets minted, and the success return parameter gets set to true.

claimOwnerIncentives would revert if !success. But in our case, it won't revert cos success is true.

This is a problem because in claimOwnerIncentives function, the rewards balance of the user has aleady been updated when calling Tokenomics#accountOwnerIncentives just before withdrawToAccount got called.

This means that the user will receive no rewards, while his reward balance gets updated.

Tools Used

Manual Review

Recommended Mitigation Steps

if amountETHFromServices< accountRewards, revert

Assessed type

Error

c4-judge commented 2 months ago

0xA5DF marked the issue as unsatisfactory: Invalid