Lack of re-entrancy protection

[code423n4]

Lines of code

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/actions/topup/TopUpAction.sol#L203 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/actions/topup/TopUpAction.sol#L275

Vulnerability details

Impact

There are many risks for re-entrancy, e.g.: function register can override the same _positions twice, because it first performs all the external calls and only then updates the state. function resetPosition first transfers the tokens to the user and only then removes the position:

  staker.decreaseActionLockedBalance(payer, position.depositTokenBalance);
  if (unstake) {
      staker.unstake(position.depositTokenBalance);
      IERC20(position.depositToken).safeTransfer(payer, position.depositTokenBalance);
  } else {
      staker.transfer(payer, position.depositTokenBalance);
  }
  _removePosition(payer, account, protocol);

function decreaseActionLockedBalance is okay if actionLockedBalances is not enough:

  if (actionLockedBalances[account] > amount) {
      actionLockedBalances[account] -= amount;
  } else {
      actionLockedBalances[account] = 0;
  }

Recommended Mitigation Steps

There are many more functions that are susceptible to re-entrancy attacks. Unfortunately this time I did not have enough time to investigate all the possible paths and impact of this, but the general idea is that you should add re-entrancy protection to your critical user facing functions that integrates with various tokens and performs external calls.

[chase-manning]

We don't plan to support any tokens that allow for re-entrancy (such as ERC777) and so the severity for this should be 2.

[gzeoneth]

Both depositToken and staker are defined within the repo without re-entrance capability. Not an issue.