Consider using assembly instead of the lengthy if statement in Router.sol

[code423n4]

Handle

hrkrshnn

Vulnerability details

Consider using assembly instead of the lengthy if statement in Router.sol

Consider the function:

function getRouterImplementation(bytes4 sig) public view returns (address)

there are several large if statements. Take for example:

if (
    sig == nTokenAction.nTokenTotalSupply.selector ||
    sig == nTokenAction.nTokenBalanceOf.selector ||
    sig == nTokenAction.nTokenTransferAllowance.selector ||
    sig == nTokenAction.nTokenTransferApprove.selector ||
    sig == nTokenAction.nTokenTransfer.selector ||
    sig == nTokenAction.nTokenTransferFrom.selector ||
    sig == nTokenAction.nTokenClaimIncentives.selector ||
    sig == nTokenAction.nTokenTransferApproveAll.selector ||
    sig == nTokenAction.nTokenPresentValueAssetDenominated.selector ||
    sig == nTokenAction.nTokenPresentValueUnderlyingDenominated.selector
) {
    return NTOKEN_ACTIONS;
}

In the above snippet, there are 10 logical ORs. Because solidity performs short circuiting for the logical or, effectively, this is same as performing 9 if statements, and therefore more expensive than required. Each if statement adds at least 20 gas (from 2 jumpi operations alone.)

This can be avoided by directly using inline assembly. Here's a rough implementation:

// The boolean value that represents `sig == nToken.... || sig == nToken... || ... || sig ==
// nToken...`
bool result = nTokenAction.nTokenTotalSupply.selector == sig;
bool tmp = sig == nTokenAction.nTokenBalanceOf.selector;
assembly { result := eq(result, tmp) }
tmp = sig == nTokenAction.nTokenTransferAllowance.selector;
assembly { result := eq(result, tmp) }
tmp = sig == nTokenAction.nTokenTransferApprove.selector;
assembly { result := eq(result, tmp) }
// and so on.
// More optimal, since it avoids branching!

Since getRouterImplementation is supposed to mimic Solidity's own function dispatch, it is worthwhile to optimize this function.

[jeffywu]

Maybe I'm not technical enough to understand this report but I ran some quick tests in remix and found some issues here:

1. The report uses eq() in the assembly statement which I don't believe is correct, that would imply an AND between result and tmp.
2. I switched the assembly to a bitwise OR and ran some tests and it appears that the suggested version uses more gas in the case when sig matches and when it does not match.

It's not clear to me from his example how his version would short circuit the comparison when it matches.

pragma solidity ^0.7.0;

contract Test {
    function getImpl1(bytes4 sig) external pure returns (bool) {
        bool result = bytes4(0x00000001) == sig;
        bool tmp = sig == 0x00000002;
        assembly { result := or(result, tmp) }

        return result;
    }

    function getImpl2(bytes4 sig) external pure returns (bool) {
        if (sig == 0x00000001 || sig == 0x00000002) {
            return true;
        }

        return false;
    }
}

[ghoul-sol]

I run similar test and this optimization does not improve gas for me. The difference is very tiny though. Making this invalid.