M-05.md

Fixed rewards in DAI (or similar token) can potentially overflow when being packed

When the lottery is initialized, fixed rewards are tightly packed in a 256 bit word. This is implemented in the packFixedRewards function:

https://github.com/code-423n4/2023-03-wenwin/blob/main/src/LotterySetup.sol#L164-L176

function packFixedRewards(uint256[] memory rewards) private view returns (uint256 packed) {
    if (rewards.length != (selectionSize) || rewards[0] != 0) {
        revert InvalidFixedRewardSetup();
    }
    uint256 divisor = 10 ** (IERC20Metadata(address(rewardToken)).decimals() - 1);
    for (uint8 winTier = 1; winTier < selectionSize; ++winTier) {
        uint16 reward = uint16(rewards[winTier] / divisor);
        if ((rewards[winTier] % divisor) != 0) {
            revert InvalidFixedRewardSetup();
        }
        packed |= uint256(reward) << (winTier * 16);
    }
}

As shown in the previous snippet, the code will first divide the rewards by 10 ** (decimals - 1), meaning it will keep only the first decimal and discard any other precision, and then cast that result as an uint16. This casting will potentially overflow if the result is bigger than the max capacity of uint16, which doesn't represent a big value considering the protocol setup.

Impact

Fixed rewards greater than 6553.5 ether will silently overflow and wrap around. If the reward token is DAI or any other similar stable coin, then 6553.5 USD can't be considered a very high value and there is a real possibility of setting a value above that, which will end up in an overflow that won't raise any error.

Proof of Concept

The following test illustrates the issue. Fixed reward for tier 1 is just on the limit (6553.5 ether) and will be ok. Reward for tier 2 (6553.6 ether) will overflow and wrap around to zero.

contract AuditTest is LotteryTestBase {
    function test_LotterySetup_packFixedRewards_FixedRewardOverflow() public {
        uint256[] memory fixedRewards = new uint256[](SELECTION_SIZE);
        fixedRewards[0] = 0;
        fixedRewards[1] = 6553.5 ether;
        fixedRewards[2] = 6553.6 ether;
        firstDrawAt = block.timestamp + 3 * PERIOD;

        Lottery lottery = new Lottery(
            LotterySetupParams(
                rewardToken,
                LotteryDrawSchedule(firstDrawAt, PERIOD, COOL_DOWN_PERIOD),
                TICKET_PRICE,
                SELECTION_SIZE,
                SELECTION_MAX,
                EXPECTED_PAYOUT,
                fixedRewards
            ),
            playerRewardFirstDraw,
            playerRewardDecrease,
            rewardsToReferrersPerDraw,
            MAX_RN_FAILED_ATTEMPTS,
            MAX_RN_REQUEST_DELAY
        );

        // Fixed rewards for tier 1 are ok...
        uint256 winTier1Reward = lottery.currentRewardSize(1);
        assertEq(winTier1Reward, fixedRewards[1]);

        // Fixed rewards for tier 2 overflowed to zero
        uint256 winTier2Reward = lottery.currentRewardSize(2);
        assertEq(winTier2Reward, 0);
    }
}

Recommendation

If the protocol decides to pack rewards in smaller types, then ensure any overflow causes an explicit error by using, for example, the OpenZeppelin library SafeCast:

function packFixedRewards(uint256[] memory rewards) private view returns (uint256 packed) {
    if (rewards.length != (selectionSize) || rewards[0] != 0) {
        revert InvalidFixedRewardSetup();
    }
    uint256 divisor = 10 ** (IERC20Metadata(address(rewardToken)).decimals() - 1);
    for (uint8 winTier = 1; winTier < selectionSize; ++winTier) {
        uint16 reward = SafeCast.toUint16(rewards[winTier] / divisor);
        if ((rewards[winTier] % divisor) != 0) {
            revert InvalidFixedRewardSetup();
        }
        packed |= uint256(reward) << (winTier * 16);
    }
}