Backd contest - pauliax's results

Originally submitted by warden pauliax in https://github.com/code-423n4/2022-04-backd-findings/issues/173, duplicate of https://github.com/code-423n4/2022-04-backd-findings/issues/52.

• .transfer is used for transfering ether, e.g.:

        payable(to).transfer(amount);
        payable(msg.sender).transfer(amount);

It is currently not recommended as recipients with custom fallback functions (smart contracts) will not be able to handle that. You can read more here: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Solution (don't forget re-entrancy protection): https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L53-L59

Originally submitted by warden pauliax in https://github.com/code-423n4/2022-04-backd-findings/issues/173, duplicate of https://github.com/code-423n4/2022-04-backd-findings/issues/49.

• Tokens having more than 18 decimals are not supported, the calculation will revert here:

  function _decimalMultiplier(address token_) internal view returns (uint256) {
      return 10**(18 - IERC20Full(token_).decimals());
  }

• Consider also introducing a _MAX_DELAY in Preparable contract to avoid situations when deadline cannot be bet due to unreasonable delay.

• There are TODOs left in the code. While this does not cause any direct issue, it indicates a bad smell and uncertainty and makes it harder for an auditor to make assumptions:

  // TODO: add constant gas consumed for transfer and tx prologue
  // TODO Add validation of curve pools
  // TODO Test validation

• Better avoid explicit casting and use a SafeCast library, e.g.:

  record.depositTokenBalance = uint128(totalLockAmount);
  position.totalTopUpAmount -= uint128(vars.totalActionTokenAmount);
  position.depositTokenBalance -= uint128(vars.depositAmountWithFees);

• SUSHISWAP is a default dex. If swapViaUniswap is not explicitly set to true for a token, then _getDex will default to _SUSHISWAP:

  function _getDex(address token_) internal view returns (UniswapRouter02) {
      return swapViaUniswap[token_] ? _UNISWAP : _SUSHISWAP;
  }

While this may be an intended behavior, I think Uniswap is more widely used and supports more pairs, so it might be a good idea to default to it instead.

You may also consider a more universal approach, e.g. token to dex mapping and general adapter for each dex to support not just these 2 dexes, but have more flexibility.

• function register in TopUpAction could have a max totalLockAmount parameter to let the users control the slippage especially when depositToken should be swapped to actionToken.

• Tokens having more than 18 decimals are not supported, the calculation will revert here:

  function _decimalMultiplier(address token_) internal view returns (uint256) {
      return 10**(18 - IERC20Full(token_).decimals());
  }

• .transfer is used for transfering ether, e.g.:

        payable(to).transfer(amount);
        payable(msg.sender).transfer(amount);

It is currently not recommended as recipients with custom fallback functions (smart contracts) will not be able to handle that. You can read more here: https://consensys.net/diligence/blog/2019/09/stop-using-soliditys-transfer-now/

Solution (don't forget re-entrancy protection): https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Address.sol#L53-L59

• In contract TopUpAction ScaledMath is used with uint128:

  using ScaledMath for uint128;

ScaledMath is written to support only uint256 type, so you shouldn't use it with lower types as they are automatically converted to uint256 and thus overflow/underflow might be escaped.

• I do not think this address(0) check is necessary here because the call to _controller.addressProvider() would have already reverted:

    constructor(IController _controller)
        Authorization(_controller.addressProvider().getRoleManager())
    {
        require(address(_controller) != address(0), Error.ZERO_ADDRESS_NOT_ALLOWED);

• controller.inflationManager() in practice can only be set once, so you can cache this value in the constructor of the StakerVault (require that it not empty address(0)). controller.addressProvider() is set in the constructor, so you could cache this too to avoid gas consuming external calls.

• Here you can just return the boolean condition:

  return _swapperImplementations[fromToken][toToken] != address(0) ? true : false;

Is the same as:

  return _swapperImplementations[fromToken][toToken] != address(0);

• Consider having an extra function in StakerVault contract that groups these functions together to avoid repeated external calls:

  stakerVault.stake(depositAmount);
  stakerVault.increaseActionLockedBalance(payer, depositAmount);

• There are some massive structs, e.g. ExecuteLocalVars. Gas efficiency can be achieved by tightly packing the struct. Struct variables are stored in 32 bytes each so you can group smaller types to occupy less storage. You can read more here: https://fravoll.github.io/solidity-patterns/tight_variable_packing.html or in the official documentation: https://docs.soliditylang.org/en/v0.4.21/miscellaneous.html

• To optimize function unstakeAndRedeem for gas usage, I would refactor it to something like this:

  unchecked {
      if (lpBalance_ < redeemLpTokens) {
          staker.unstakeFor(msg.sender, msg.sender, redeemLpTokens - lpBalance_);
      }
  }

  uint256 lpBalance_ = lpToken.balanceOf(msg.sender);
  require(lpBalance_ >= redeemLpTokens, Error.INSUFFICIENT_BALANCE);

  return redeem(redeemLpTokens, minRedeemAmount);