setBooster() function may be used to steal unclaimed rewards in FlywheelCore contract

[code423n4]

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/base/FlywheelCore.sol#L137-L141

Vulnerability details

Lines of code

https://github.com/code-423n4/2023-05-maia/blob/main/src/rewards/base/FlywheelCore.sol#L137-L141

Vulnerability details

Impact

A malicious owner can steal all unclaimed rewards and break the reward accounting mechanism

Proof of Concept

Even if the owner is a good guy but the fact that there exists a rug vector available may negatively impact the protocol's reputation. Or maybe the hotkeys of a multi-sig wallet may be stolen. Furthermore, since this contract is meant to be used by other projects, the trustworthiness of every project cannot be vouched for.

The problem lies in the fact that the flywheelRewards is not immutable. Let's check a hypothetical process a malicious owner can take. The boostedBalanceOf() function is used to calculate the boosted balance of a user in a given strategy. By creating and setting a booster contract that returns zero when users call boostedBalanceOf() in a situation where the user address is not under the attacker's control, and returning arbitrary values for those under his/her control, an attacker can choose specific amounts of rewardToken to assign to himself/herself. The attacker can then call claimRewards() to withdraw the funds. Any amounts that the attacker assigns to himself/herself over the amount that normally would have been assigned, upon claiming, is taken from other users' unclaimed balances since tokens are custodied by the flywheelRewards address rather than per-user accounts.

    function setBooster(IFlywheelBooster newBooster) external onlyOwner {
        flywheelBooster = newBooster;

        emit FlywheelBoosterUpdate(address(newBooster));
    }

In the function accrueUser() we have the supplierToken variable defined in a way that calls boostedBalanceOf():

    function accrueUser(ERC20 strategy, address user, uint256 index) private returns (uint256) {
        // load indices
        uint256 supplierIndex = userIndex[strategy][user];

        // sync user index to global
        userIndex[strategy][user] = index;

        // if user hasn't yet accrued rewards, grant them interest from the strategy beginning if they have a balance
        // zero balances will have no effect other than syncing to global index
        if (supplierIndex == 0) {
            supplierIndex = ONE;
        }

        uint256 deltaIndex = index - supplierIndex;
        // use the booster or token balance to calculate reward balance multiplier
        uint256 supplierTokens = address(flywheelBooster) != address(0)
            ? flywheelBooster.boostedBalanceOf(strategy, user)
            : strategy.balanceOf(user);

        // accumulate rewards by multiplying user tokens by rewardsPerToken index and adding on unclaimed
        uint256 supplierDelta = (supplierTokens * deltaIndex) / ONE;
        uint256 supplierAccrued = rewardsAccrued[user] + supplierDelta;

        rewardsAccrued[user] = supplierAccrued;

        emit AccrueRewards(strategy, user, supplierDelta, index);

        return supplierAccrued;
    }

Finally, the function claimRewards() is defined in the preceding way:

    function claimRewards(address user) external {
        uint256 accrued = rewardsAccrued[user];

        if (accrued != 0) {
            rewardsAccrued[user] = 0;

            rewardToken.safeTransferFrom(address(flywheelRewards), user, accrued);

            emit ClaimRewards(user, accrued);
        }
    }

Tools Used

Manual Review https://twitter.com/RugDocIO/status/1411732108029181960 https://twitter.com/Mudit__Gupta/status/1675584195798913024

Recommended Mitigation Steps

Make flywheelRewards address immutable, or only allow it to change if there are no current users

Assessed type

Decimal

[c4-judge]

trust1995 marked the issue as unsatisfactory: Out of scope

[trust1995]

Belongs in Centralization risks report