code-423n4 / 2022-02-tribe-turbo-findings

1 stars 0 forks source link

Gas Optimizations #22

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

There should be a safe.slurpAndLess() in order to avoid duplicating the modifiers of the separate functions

    function slurpAndLess(TurboSafe safe, ERC4626 vault, uint256 feiAmount) external authenticate(address(safe)) {
        safe.slurp(vault);
        safe.less(vault, feiAmount);
    }

https://github.com/code-423n4/2022-02-tribe-turbo/blob/66f27fe51083f49f7935e3fe594ab2380b75dee8/src/TurboRouter.sol#L130-L133

Functions not called by the contract itself should be external rather than public

All of the rest in this contract are external, and this one isn't called anywhere

    function boost(TurboSafe safe, ERC4626 vault, uint256 feiAmount) public authenticate(address(safe)) {
        safe.boost(vault, feiAmount);
    }

https://github.com/code-423n4/2022-02-tribe-turbo/blob/66f27fe51083f49f7935e3fe594ab2380b75dee8/src/TurboRouter.sol#L118

Splitting require() statements that use && saves gas

See this issue for an example

require(getTotalFeiBoostedForVault[ERC4626(address(token))] == 0 && token != assetTurboCToken, "INVALID_TOKEN");        

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/TurboSafe.sol

require(success && deployed.code.length != 0, "INITIALIZATION_FAILED");          

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/lib/solmate/src/utils/CREATE3.sol (technically not in scope but maybe useful)

require(recoveredAddress != address(0) && recoveredAddress == owner, "INVALID_SIGNER");        

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/lib/solmate/src/tokens/ERC20.sol (technically not in scope but maybe useful)

require(success && deployed.code.length != 0, "INITIALIZATION_FAILED");          

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/lib/ERC4626/lib/solmate/src/utils/CREATE3.sol (technically not in scope but maybe useful)

require(recoveredAddress != address(0) && recoveredAddress == owner, "INVALID_SIGNER");        

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/lib/ERC4626/lib/solmate/src/tokens/ERC20.sol (technically not in scope but maybe useful)

Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.
bool public frozen;             

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/src/modules/TurboBooster.sol#L28

<array>.length should not be looked up in every loop of a for-loop

for (uint256 i = 0; i < data.length; i++) {      

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/lib/ERC4626/src/external/Multicall.sol#L14 (technically not in scope but maybe useful)

++i/i++ should be unchecked{++i}/unchecked{++i} when it is not possible for them to overflow, as is the case when used in for- and while-loops

for (uint256 i = 0; i < data.length; i++) {      

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/lib/ERC4626/src/external/Multicall.sol#L14 (technically not in scope but maybe useful)

++i costs less gas than ++i, especially when it's used in for-loops (--i/i-- too)

for (uint256 i = 0; i < data.length; i++) {      

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/lib/ERC4626/src/external/Multicall.sol#L14 (technically not in scope but maybe useful)

It costs more gas to initialize variables to zero than to let the default of zero be applied

for (uint256 i = 0; i < data.length; i++) {      

https://github.com/code-423n4/2022-02-tribe-turbo/blob/main/lib/ERC4626/src/external/Multicall.sol#L14 (technically not in scope but maybe useful)

GalloDaSballo commented 2 years ago

I have to preface that I very much like the formatting on this submission. I think all submission should try to show the code and have titles, it does help.

safe.slurpAndLess() can't argue with that, would save a CALL and a Auth check, let's say 200 gas

Removing && would save 3 gas per &, let's say 15 gas

for (uint256 i = 0; i < data.length; i++) { in multicall is indeed paying the extra cost for getting the length each time, (boundary check) caching it would save gas, let's say 10 (because out of scope)

Unfortunately everything else is out of scope although technically valid

I want to commend the warden for their submission and the thoroughness

225 gas saved