Infinity NFT Marketplace contest - wagmi's results

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L303

Vulnerability details

Impact

Function _updateUserStakedAmounts() of InfinityStaker is use to update amount and timestamp of user’s staked funds when user call unstake(). But instead of using staked amount to check, it uses vested amount to check (amount > vestedThreeMonths) and set amount to 0, which means user’s staked funds is lost completely.

Proof of Concept

1. Alice staked 1000 tokens for 12 months.
2. After 12 months, she staked 2000 tokens for 3 months.
3. After 1 month, now she should be able to unstake 1000 tokens because it has been staked for 13 months. But when she calls unstake(1000), she also loses her 2000 tokens staked for 3 months.

For more detail, please check this test. You can add it to staker.js to run

it.only('lose funds when unstake', async function () {
      // approve erc20
      await approveERC20(signer1.address, token.address, amountStaked2, signer1, infinityStaker.address);

      // stake 1000 token for 12 months
      await infinityStaker.connect(signer1).stake(toBN(1000), 3);
      
      // increase time by 1 year
      await network.provider.send('evm_increaseTime', [12 * MONTH + 1 * DAY]);
      await network.provider.send('evm_mine', []);

      // stake 2000 token for 3 months
      await infinityStaker.connect(signer1).stake(toBN(2000), 1);
      
      // check current staked fund
      expect(await infinityStaker.getUserTotalStaked(signer1.address)).to.equal(toBN(3000));

      // unstake 1000 token
      await infinityStaker.connect(signer1).unstake(toBN(1000));

      // check current staked fund
      expect(await infinityStaker.getUserTotalStaked(signer1.address)).to.equal(toBN(2000));
});

Tools Used

Manual Review

Recommended Mitigation Steps

Use staked amount instead of vested amount in _updateUserStakedAmounts() function.

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/765376fa238bbccd8b1e2e12897c91098c7e5ac6/contracts/staking/InfinityStaker.sol#L210

Vulnerability details

Impact

Function getUserStakeLevel in InfinityStaker should return level of user given staked amount and staked threshold. But this function assumes that stake thresholds is ascending and will have unexpected behaviours if it’s not.

This can happen when owner mistakenly set stake thresholds updateStakeLevelThreshold()

Proof of Concept

For example, if stake thresholds is [1000, 12000, 10000, 20000] with level [BRONZE, SILVER, GOLD, PLATINUM] respectively. Alice's stake power is 11000. She should have GOLD level but becauce it checks low level first then she will only receive SILVER level

Tools Used

Manual review

Recommended Mitigation Steps

Should add check to make sure stake thresholds is always ascending.

Or in getUserStakeLevel can check high level first (e.g check PLATINUM -> GOLD -> … -> BRONZE)