code-423n4 / 2021-09-yaxis-findings

0 stars 0 forks source link

Change public visibility to external #55

Open code423n4 opened 3 years ago

code423n4 commented 3 years ago

Handle

0xRajeev

Vulnerability details

Impact

There are some public visibility calls across contracts that could be made external because they are not called from within contracts. Public visibility forces a copy of function parameters from calldata to memory which consumes gas depending on the number of parameters.

Proof of Concept

Example: https://github.com/code-423n4/2021-09-yaxis/blob/cf7d9448e70b5c1163a1773adb4709d9d6ad6c99/contracts/v3/Harvester.sol#L297-L302

Tools Used

Manual Analysis

Recommended Mitigation Steps

Evaluate all public visibility functions and set to external if they are not called from within contracts.

uN2RVw5q commented 3 years ago

Public visibility forces a copy of function parameters from calldata to memory which consumes gas depending on the number of parameters

This is incorrect. I think it was true for very old solidity compiler versions. calldata and memory have to be now explicitly specified.

The change would not affect gas. But the change would improve code quality / style.

I agree with "sponsor disputed" tag since the issue was submitted as a gas optimization, but changing this to a coding style issue is also reasonable.

GalloDaSballo commented 3 years ago

Agree with finding, the finding claims that some public functions could be changed to external

Would love to see demonstration of the gas statements, however, changing visibility is a legitimate way of improving code-quality

non-critical