Backd contest - Tadashi's results

Some tokens require the approval to be zero before calling approve with a non-zero value

Summary: some ERC20 tokens require approving to 0 first

Details: Some tokens (such as USDT) do not work when changing the allowance from an existing non-zero allowance value.

Mitigation: In L50 of TopUpAction.sol change the code to

IERC20(token).safeApprove(stakerVaultAddress, 0);
IERC20(token).safeApprove(stakerVaultAddress, depositAmount);

The same pattern should be applied to the following lines mutatis mutandis:

• L847 of TopUpAction.sol
• L908 of TopUpAction.sol
• L209 of StrategySwapper.sol
• L21 of Erc20Vault.sol
• L22 of Erc20Vault.sol

Impact: Low, depending which tokens will be integrated in backd

Misleading Error message

Summary: misleading error message could potentially confuse developers when debugging code

Details:

On L86 of Preparable.sol, if deadlines[key] is zero then the return message will be Error.DEADLINE_NOT_ZERO, which is the opposite condition of the error (cf. L28 of Preparable.sol for the same error but with the correct context).

The same can be said of L98 of Preparable.sol.

Impact: Code QA

Typos

• palce instead of place in Preparable.sol

TODOs left in the code

• L4 of ConvexStrategyBase.sol
• L5 of ConvexStrategyBase.sol
• L713 of TopUpAction.sol: this one is essential for not underestimating gas costs in user´s gas bank

Immutable variable can be constant

Summary: Immutable variable can be changed to constant to reduce deployment cost

Details: Immutable variables are evaluated once at construction time and their value is copied to all the places in the code where they are accessed. For these values, 32 bytes are reserved, even if they would fit in fewer bytes. In particular address variables has only 20 bytes and the immutable variable of L27 of ChainlinkUSDWrapper.sol can be changed to a constant variable, i.e.:

IChainlinkOracle private constant _ethOracle =
        IChainlinkOracle(0x5f4eC3Df9cbd43714FE2740f5E3616155c5b8419);

Remove branch not being executed

Summary: some branches are not executed and can be eliminated from the code

Details:

• Function addStakerVault (L277 of AddressProvider.sol) only reverts or return true. This means that L43-L45 of Controller.sol can be simplified to:

  addressProvider.addStakerVault(stakerVault);

Split require to avoid using && opcode

Details: Change L676 of TopUpAction.sol to

require(vars.success, Error.TOP_UP_FAILED);
require(abi.decode(vars.topupResult, (bool)), Error.TOP_UP_FAILED);

in order to avoid using &&.

Note: I only recommend this optimization if backd gives a proper documentation for the error Error.TOP_UP_FAILED. Otherwise, developers may miss one of the conditions when debugging a reverted transaction.

Pre-incrementing a variable is cheaper than post-incrementing it.

Details: i++ can be replaced by ++i in the following loops:

• L117 of Controller.sol
• L260 of StakerVault.sol
• L80 of RoleManager.sol
• L188 of TopUpAction.sol
• L456 of TopUpAction.sol
• L479 of TopUpAction.sol
• L506 of TopUpAction.sol
• L891 of TopUpAction.sol
• L43 of TopUpKeeperHelper.sol
• L72 of TopUpKeeperHelper.sol
• L93 of TopUpKeeperHelper.sol
• L165 of TopUpKeeperHelper.sol
• L135 of CompoundHandler.sol
• L61 of CTokenRegistry.sol
• L313 of ConvexStrategyBase.sol
• L380 of ConvexStrategyBase.sol