Gas optimization on the public functions

[code423n4]

Handle

defsec

Vulnerability details

Impact

In a nutshell, public and external differs in terms of gas usage. The former use more than the latter when used with large arrays of data. This is due to the fact that Solidity copies arguments to memory on a public function while external read from calldata which a cheaper than memory allocation. public functions are called internally and externally. internal calls are executed via jumps in the code because array arguments are passed internally by pointers to memory. When the compiler generates the code for an internal function, that function expects its arguments to be located in memory. That is why public functions are allocated to memory. The optimization that happens with external is that is does not care for the internal calls. So if you know the function you create only allows for external calls, go for it. It provides performance benefits and you will save on gas. In the Sushi Trident contracts, some of the methods are marked as public. The public function should be replaced with the external ones.

Proof of Concept

1. Navigate to "contracts/pool/HybridPool.sol" contract on the trident repository. (https://github.com/sushiswap/trident/blob/master/contracts/pool/HybridPool.sol)

The following functions are marked as a public.

burnSingle(),swap(),flashSwap(),getAmountOut(),getAssets(),mint() , burn() 

2. Navigate to "contracts/pool/ConstantProductPool.sol" contract on the trident repository. (https://github.com/sushiswap/trident/blob/9130b10efaf9c653d74dc7a65bde788ec4b354b5/contracts/pool/ConstantProductPool.sol)

The following functions are marked as a public.

burnSingle(),swap(),flashSwap(),getAmountOut(),getAssets(),mint() , burn() 

3. Navigate to "contracts/pool/IndexPool.sol" contract on the trident repository. (https://github.com/sushiswap/trident/blob/master/contracts/pool/IndexPool.sol)

The following functions are marked as a public.

burnSingle(),swap(),flashSwap(),getAmountOut(),getAssets(),mint() , burn() 

POC

pragma solidity^0.4.12;

contract Test {
    function test(uint[20] a) public returns (uint){
         return a[10]*2;
    }

    function test2(uint[20] a) external returns (uint){
         return a[10]*2;
    }
}

Calling each function, we can see that the public function uses 496 gas, while the external function uses only 261.

Tools Used

None

Recommended Mitigation Steps

Overrided function should be marked as an external for the gas optimization.

public - all can access

external - Cannot be accessed internally, only externally

internal - only this contract and contracts deriving from it can access

private - can be accessed only from this contract

[maxsam4]

The reason that external modifier can sometimes be cheaper than public is that Solidity keeps function parameters in calldata in external but copies them to memory in public. If there are no function parameters in a function or they are explicitly set to be kept in calldata, there is no potential gas saving. This is the case in our contracts and hence, changing public to external won't do anything.

[alcueca]

Agree with sponsor