search

code-423n4 / 2022-01-xdefi-findings

0 stars 0 forks source link

Use of return value from assignment hampers readability #2

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

TomFrenchBlockchain

Vulnerability details

Impact

Reduced readability

Proof of Concept

In a number of placed we seem to be inlining an assignment with the usage of that variable:

https://github.com/XDeFi-tech/xdefi-distribution/blob/3856a42df295183b40c6eee89307308f196612fe/contracts/XDEFIDistribution.sol#L40

https://github.com/XDeFi-tech/xdefi-distribution/blob/3856a42df295183b40c6eee89307308f196612fe/contracts/XDEFIDistribution.sol#L70

https://github.com/XDeFi-tech/xdefi-distribution/blob/3856a42df295183b40c6eee89307308f196612fe/contracts/XDEFIDistribution.sol#L83

This is quite atypical in my experience and reduces readability: lines which contain require statements and event emission now modify contract storage.

Recommended Mitigation Steps

Consider whether any small benefits to gas/compactness are worth the reduced clarity.

deluca-mike commented 2 years ago

Agreed. Will go with the option to assign return variables in order to keep the function prototype pattern/style consistent (i.e. return variables all have names).

deluca-mike commented 2 years ago

You can see in the release candidate contract that these are all fixed, but specifically: