Infinity NFT Marketplace contest - p4st13r4's results

Lines of code

https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L290

Vulnerability details

Impact

Affected code:

• https://github.com/code-423n4/2022-06-infinity/blob/main/contracts/staking/InfinityStaker.sol#L290

The unstake() function of InfinityStaker.sol receives an amount and computes the vested amount starting from the lowest staking tier (NONE) to the highest staking tier (TWELVE_MONTHS). This logic is flawed because it’s based on the assumption that users will stake linearly, following this tier, and will change duration of what they have staked using the builtin changeDuration().

However, if a user stakes an amount for e.g. 6 months, and then at the end of it she tries to unstake the portion that has vested, she will lose all the staked amounts of the inferior tiers due to wrong accounting implemented in the InfinityStaker.sol contract.

Proof of Concept

Replace the last test in test/staker.js with the following one:

it('Should succeed', async function () {
  // approve erc20
  await approveERC20(signer1.address, token.address, amountStaked2, signer1, infinityStaker.address);

  // stake to 6 months
  totalStaked = amountStaked2;
  await infinityStaker.connect(signer1).stake(amountStaked2, 2);
  expect(await infinityStaker.getUserTotalStaked(signer1.address)).to.equal(totalStaked);
  expect(await infinityStaker.getUserTotalVested(signer1.address)).to.equal(0);

  // increase time to 6 months and 1 day
  await network.provider.send('evm_increaseTime', [181 * DAY]);
  await network.provider.send('evm_mine', []);

  // stake to 3 months
  totalStaked = totalStaked.add(amountStaked2);
  await infinityStaker.connect(signer1).stake(amountStaked2, 1);
  expect(await infinityStaker.getUserTotalStaked(signer1.address)).to.equal(totalStaked);
  expect(await infinityStaker.getUserTotalVested(signer1.address)).to.equal(amountStaked2);

  // unstake half of the total staked, corresponding to the first tranche that is now unlocked
  let totalVested = amountStaked2;
  await infinityStaker.unstake(totalVested);
  totalStaked = totalStaked.sub(totalVested);
  
  // @audit test fails with: AssertionError: Expected "0" to be equal 5000000000000000000000
  expect(await infinityStaker.getUserTotalStaked(signer1.address)).to.equal(totalStaked);
});

This test performs the following steps:

• Alice stakes 5000 for 6 months
• After 6 months, Alice stakes another 5000 for 3 months
• Alice then unstakes 5000
• Alice should receive 5000, and still have 5000 staked
• Instead, Alice receives 5000 but infinityStaker.getUserTotalStaked() returns 0

Tools Used

Editor

Recommended Mitigation Steps

The unstake() function should receive a duration along with an amount param, so users can unstake from a specific tier.