Slingshot Finance contest - WatchPug's results

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-10-slingshot/blob/9c0432cca2e43731d5a0ae9c151dacf7835b8719/contracts/Slingshot.sol#L108-L114

/// @notice Sets module registry used to verify modules
/// @param _moduleRegistry The address of module registry
function setModuleRegistry(address _moduleRegistry) external onlyAdmin {
    address oldRegistry = address(moduleRegistry);
    moduleRegistry = ModuleRegistry(_moduleRegistry);
    emit NewModuleRegistry(oldRegistry, _moduleRegistry);
}

The event NewModuleRegistry can be emitted earlier to get rid of the local variable oldRegistry and save some gas.

This change will also make the setModuleRegistry() function be more consistent with setApprovalHandler().

Recommendation

Change to:

function setModuleRegistry(address _moduleRegistry) external onlyAdmin {
    emit NewModuleRegistry(moduleRegistry, _moduleRegistry);
    moduleRegistry = ModuleRegistry(_moduleRegistry);
}

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-10-slingshot/blob/9c0432cca2e43731d5a0ae9c151dacf7835b8719/contracts/ModuleRegistry.sol#L35-L64

/// @param _moduleAddress Address of the module to register
function registerSwapModule(address _moduleAddress) public onlyAdmin {
    require(!modulesIndex[_moduleAddress], "oops module already exists");
    require(ISlingshotModule(_moduleAddress).slingshotPing(), "not a module");

    modulesIndex[_moduleAddress] = true;
    emit ModuleRegistered(_moduleAddress);
}

/// @param _moduleAddresses Addresses of modules to register
function registerSwapModuleBatch(address[] memory _moduleAddresses) external onlyAdmin {
    for (uint256 i = 0; i < _moduleAddresses.length; i++) {
        registerSwapModule(_moduleAddresses[i]);
    }
}

/// @param _moduleAddress Address of the module to unregister
function unregisterSwapModule(address _moduleAddress) public onlyAdmin {
    require(modulesIndex[_moduleAddress], "module does not exist");

    delete modulesIndex[_moduleAddress];
    emit ModuleUnregistered(_moduleAddress);
}

/// @param _moduleAddresses Addresses of modules to unregister
function unregisterSwapModuleBatch(address[] memory _moduleAddresses) external onlyAdmin {
    for (uint256 i = 0; i < _moduleAddresses.length; i++) {
        unregisterSwapModule(_moduleAddresses[i]);
    }
}

registerSwapModuleBatch() and unregisterSwapModuleBatch() already got onlyAdmin check, and registerSwapModule() will check it again multiple times.

Consider creating two private functions without access control and call them inside the public functions.

Recommendation

Change to:

function _registerSwapModule(address _moduleAddress) private {
    require(!modulesIndex[_moduleAddress], "oops module already exists");
    require(ISlingshotModule(_moduleAddress).slingshotPing(), "not a module");

    modulesIndex[_moduleAddress] = true;
    emit ModuleRegistered(_moduleAddress);
}

/// @param _moduleAddress Address of the module to register
function registerSwapModule(address _moduleAddress) public onlyAdmin {
    _registerSwapModule(_moduleAddress);
}

/// @param _moduleAddresses Addresses of modules to register
function registerSwapModuleBatch(address[] memory _moduleAddresses) external onlyAdmin {
    for (uint256 i = 0; i < _moduleAddresses.length; i++) {
        _registerSwapModule(_moduleAddresses[i]);
    }
}

function _unregisterSwapModule(address _moduleAddress) private {
    require(modulesIndex[_moduleAddress], "module does not exist");

    delete modulesIndex[_moduleAddress];
    emit ModuleUnregistered(_moduleAddress);
}

/// @param _moduleAddress Address of the module to unregister
function unregisterSwapModule(address _moduleAddress) public onlyAdmin {
    _unregisterSwapModule(_moduleAddress);
}

/// @param _moduleAddresses Addresses of modules to unregister
function unregisterSwapModuleBatch(address[] memory _moduleAddresses) external onlyAdmin {
    for (uint256 i = 0; i < _moduleAddresses.length; i++) {
        _unregisterSwapModule(_moduleAddresses[i]);
    }
}

Handle

WatchPug

Vulnerability details

UniswapModule.sol can be changed into a regular (non-abstract) contract. And add an immutable variable router and set the value for it in the constructor function.

https://github.com/code-423n4/2021-10-slingshot/blob/9c0432cca2e43731d5a0ae9c151dacf7835b8719/contracts/module/IUniswapModule.sol#L34-L34

address router = getRouter();

Then, L34 and getRouter() can be removed and save 1 internal call and reduce the code size.

SushiSwapModule.sol and UniswapModule.sol will no longer be needed as the router can be configurated at deployment.

Recommendation

Change to:

contract UniV2Module is ISlingshotModule {
    using LibERC20Token for IERC20;

    address public immutable router;
    
    constructor (address _router) {
        router = _router;
    }

    /// @param amount Amount of the token being exchanged
    /// @param path Array of token addresses to swap
    /// @param tradeAll If true, it overrides totalAmountIn with current token balance
    function swap(
        uint256 amount,
        address[] memory path,
        bool tradeAll
    ) external payable {
        require(path.length > 0, "UniswapModule: path length must be >0");

        if (tradeAll) {
            amount = IERC20(path[0]).balanceOf(address(this));
        }

        IERC20(path[0]).approveIfBelow(router, amount);

        // for now, we only supporting .swapExactTokensForTokens()
        // amountOutMin is 1, because all we care is final check or output in Slingshot contract
        IUniswapV2Router02(router).swapExactTokensForTokensSupportingFeeOnTransferTokens(
            amount,
            1, // amountOutMin
            path,
            address(this),
            block.timestamp
        );
    }
}

Handle

WatchPug

Vulnerability details

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Instances include:

• ModuleRegistry.sol#registerSwapModuleBatch()

  https://github.com/code-423n4/2021-10-slingshot/blob/9c0432cca2e43731d5a0ae9c151dacf7835b8719/contracts/ModuleRegistry.sol#L46-L46

• ModuleRegistry.sol#unregisterSwapModuleBatch()

  https://github.com/code-423n4/2021-10-slingshot/blob/9c0432cca2e43731d5a0ae9c151dacf7835b8719/contracts/ModuleRegistry.sol#L61-L61

• Executioner.sol#executeTrades()

  https://github.com/code-423n4/2021-10-slingshot/blob/9c0432cca2e43731d5a0ae9c151dacf7835b8719/contracts/Executioner.sol#L34-L34

• Slingshot.sol#executeTrades()

  https://github.com/code-423n4/2021-10-slingshot/blob/9c0432cca2e43731d5a0ae9c151dacf7835b8719/contracts/Slingshot.sol#L75-L76

Handle

WatchPug

Vulnerability details

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

For example:

https://github.com/code-423n4/2021-10-slingshot/blob/9c0432cca2e43731d5a0ae9c151dacf7835b8719/contracts/lib/LibERC20Token.sol#L18-L32

if (currentAllowance < amount) {
    // we can optimise gas by using uint256(-1) instead of `amount`
    // using amount is safer, but then each time we interacting with token
    // we have to approve it.
    // For now, let's go with more secure option,
    // when we add whitelisting for tokens and revoke "super admin" option for updating modules
    // we can go for gas and change it to uint256.max
    //
    // however gas usage base on tests looks like this:
    // - with amount:       min 126003, max 442559, avg 249189
    // - with uint256(-1):  min 141006, max 499514, avg 277865
    // so we will spend more at first run (when we need to save all uint256 bits),
    // but then we will gain in next runs.
    token.safeIncreaseAllowance(spender, amount - currentAllowance);
}

amount - currentAllowance will never underflow.

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-10-slingshot/blob/9c0432cca2e43731d5a0ae9c151dacf7835b8719/contracts/ModuleRegistry.sol#L66-L74

/// @param _slingshot Slingshot.sol address implementation
function setSlingshot(address _slingshot) external onlyAdmin {
    require(_slingshot != address(0x0), "slingshot is empty");
    require(slingshot != _slingshot, "no changes to slingshot");

    address oldAddress = slingshot;
    slingshot = _slingshot;
    emit NewSlingshot(oldAddress, _slingshot);
}

Swap the order of L71 and L69 and use oldAddress to compare with _slingshot can save 1 storage read.

Recommendation

Change to:

function setSlingshot(address _slingshot) external onlyAdmin {
    require(_slingshot != address(0x0), "slingshot is empty");
    address oldAddress = slingshot;
    require(oldAddress != _slingshot, "no changes to slingshot");
    
    slingshot = _slingshot;
    emit NewSlingshot(oldAddress, _slingshot);
}

Handle

WatchPug

Vulnerability details

Every reason string takes at least 32 bytes.

Use short reason strings that fit in 32 bytes or it will become more expensive.

Instances include:

https://github.com/code-423n4/2021-10-slingshot/blob/9c0432cca2e43731d5a0ae9c151dacf7835b8719/contracts/ApprovalHandler.sol#L15

require(isSlingshot(_msgSender()), "Adminable: not a SLINGSHOT_CONTRACT_ROLE");

https://github.com/code-423n4/2021-10-slingshot/blob/9c0432cca2e43731d5a0ae9c151dacf7835b8719/contracts/Slingshot.sol#L74

require(finalAmountMin > 0, "Slingshot: finalAmountMin cannot be zero");

https://github.com/code-423n4/2021-10-slingshot/blob/9c0432cca2e43731d5a0ae9c151dacf7835b8719/contracts/Slingshot.sol#L93

require(finalOutputAmount >= finalAmountMin, "Slingshot: result is lower than required min");

Handle

WatchPug

Vulnerability details

https://github.com/code-423n4/2021-10-slingshot/blob/9c0432cca2e43731d5a0ae9c151dacf7835b8719/contracts/Slingshot.sol#L76-L79

for(uint256 i = 0; i < trades.length; i++) {
    // Checks to make sure that module exists and is correct
    require(moduleRegistry.isModule(trades[i].moduleAddress), "Slingshot: not a module");
}

An external call to moduleRegistry.isModule() will be called each time in this for loop. They can be combined into one external call by creating an moduleRegistry.isModuleBatch(address[] memory _moduleAddresses) function and call that function instead.

Recommendation

Change to:

address[] memory moduleAddresses = new address[](trades.length);
for(uint256 i = 0; i < trades.length; i++) {
    moduleAddresses[i] = trades[i].moduleAddress;
}
require(moduleRegistry.isModuleBatch(moduleAddresses), "Slingshot: not a module");