Backd contest - Funen's results

1. Comment was not same as actual code

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/pool/LiquidityPool.sol#L323

the current implementation was set return true, so it has not the same as comment do. cause it was return true if address of the new staker vault for the pool was correct.

##Tool Used Manual Review

##Recommended Mitigation Change it or remove it

##Another Occurances

1.StakerVault 2.TopUpKeeperHelper

2. Missbehavior isShutdown in shutdown()

Since isShutdown was return value false, it would be missbehavior executed after. cause return was false. This implementation usually or common use using logical operator ! for isShutdown below can be set for good or you can set it into modifier for is it done or not.

##Tool Used Manual Review

##POC Using this logic https://www.tabnine.com/code/java/methods/io.netty.util.concurrent.EventExecutor/isShutdown

##Recommended Mitigation function shutdown() external override onlyVault returns (bool) { if (!isShutdown) return false; isShutdown = true; emit Shutdown(); return true; }

or you can using this logic :

contract Shutdownable is Ownable { bool public isShutdown;

event Shutdown();

modifier notShutdown {
    require(!isShutdown, "Smart contract is shut down.");
    _;
}

function shutdown() public onlyOwner {
    isShutdown = true;
    emit Shutdown();
}

}

1. using ++i than i++ for saving more gas

Using i++ instead ++i for all the loops, the variable i is incremented using i++. It is known that implementation by using ++i costs less gas per iteration than i++.

Tools Used

Manual Review

Occurances

contracts/actions/topup/TopUpAction.sol#L188
contracts/actions/topup/TopUpAction.sol#L456
contracts/actions/topup/TopUpAction.sol#L479
contracts/actions/topup/TopUpAction.sol#L506
contracts/actions/topup/TopUpAction.sol#L891
contracts/StakerVault.sol#L260
contracts/strategies/ConvexStrategyBase.sol#L313
contracts/strategies/ConvexStrategyBase.sol#L380
contracts/actions/topup/handlers/CTokenRegistry.sol#L61
contracts/actions/topup/TopUpKeeperHelper.sol#L43
contracts/actions/topup/TopUpKeeperHelper.sol#L72
contracts/actions/topup/TopUpKeeperHelper.sol#L93
contracts/actions/topup/TopUpKeeperHelper.sol#L165

2. change uint256 i = 0 into uint i for saving more gas

using this implementation can saving more gas for each loops.

##Tool Used Manual Review & Remix

##Recommended Mitigation Change it

##Occurances

contracts/actions/topup/TopUpAction.sol#L188
contracts/actions/topup/TopUpAction.sol#L456
contracts/actions/topup/TopUpAction.sol#L479
contracts/actions/topup/TopUpAction.sol#L506
contracts/actions/topup/TopUpAction.sol#L891
contracts/StakerVault.sol#L260
contracts/strategies/ConvexStrategyBase.sol#L313
contracts/strategies/ConvexStrategyBase.sol#L380
contracts/actions/topup/handlers/CTokenRegistry.sol#L61
contracts/actions/topup/TopUpKeeperHelper.sol#L43
contracts/actions/topup/TopUpKeeperHelper.sol#L72
contracts/actions/topup/TopUpKeeperHelper.sol#L93
contracts/actions/topup/TopUpKeeperHelper.sol#L165

3. Set Value to immutable for saving more gas

this can be set as immutable for saving more gas

##Tool Used Remix

##Recommended Mitigation add immutable

##Occurances

ConvexStrategyBase.sol

main/backd/contracts/strategies/ConvexStrategyBase.sol#L47
main/backd/contracts/strategies/ConvexStrategyBase.sol#L56
main/backd/contracts/strategies/ConvexStrategyBase.sol#L58
main/backd/contracts/strategies/ConvexStrategyBase.sol#L59

4. Saving gas by removing = 0

If a variable was not set/initialized, it is assumed to have default value to 0 this implementation was used for saving more gas by removing = 0

##TOOLS USED Remix, Manual Review

##Mitigation Step Remove = 0

##Occurances

contracts/actions/topup/TopUpAction.sol#L63
contracts/pool/LiquidityPool.sol#L389
contracts/StakerVault.sol#L144
contracts/Controller.sol#L114

5. Better way of using safeERC20 for saving more gas

This implementation can be used to saving more gas instead.

##POC https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#SafeERC20-safeApprove-contract-IERC20-address-uint256-

##Recommended Mitigation

by not declaring :

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

using SafeERC20 for IERC20;

and changed to :

            SafeERC20.safeTransferFrom(IERC20(token), payer, address(this), depositAmount);
            SafeERC20.safeApprove(IERC20(token),stakerVaultAddress, depositAmount);

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpAction.sol#L49-L50

##Another Occurances

The same method can be used for this contract

StrategySwapper.sol
ConvexStrategyBase.sol
Erc20Pool.sol
Erc20Vault.sol
VaultReserve.sol
CvxCrvRewardsLocker.sol

6. Caching in memory can be saving more gas

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/pool/LiquidityPool.sol#L779

in this line since was used for read only, it can be saving by caching in memory instead of using storage

##Tool Used Manual Review, Remix

7. Used > instead of >= for saving more gas

This current implementation indeed can saving more gas

##Tool Used Remix

##Occurances

contracts/StakerVault.sol#L107
contracts/StakerVault.sol#L148
contracts/StakerVault.sol#L153
contracts/GasBank.sol#L68

8. Used `++topupsAdded`` can saving more gas

https://github.com/code-423n4/2022-04-backd/blob/c856714a50437cb33240a5964b63687c9876275b/backd/contracts/actions/topup/TopUpKeeperHelper.sol#L50

This implementation using ++topupsAdded can be saving more gas instead.

##Tool Used Remix