code-423n4 / 2021-10-ambire-findings

0 stars 0 forks source link

Inconsistent code style of for loops #45

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

Most of the for loops in the codebase use < to control the loop:

for (uint i=0; i<len; i++) {

However, in Zapper.sol, all 7 for loops are using !=:

https://github.com/code-423n4/2021-10-ambire/blob/bc01af4df3f70d1629c4e22a72c19e6a814db70d/contracts/wallet/Zapper.sol#L79-L79

for (uint i=0; i!=spenders.length; i++) {

https://github.com/code-423n4/2021-10-ambire/blob/bc01af4df3f70d1629c4e22a72c19e6a814db70d/contracts/wallet/Zapper.sol#L87-L87

https://github.com/code-423n4/2021-10-ambire/blob/bc01af4df3f70d1629c4e22a72c19e6a814db70d/contracts/wallet/Zapper.sol#L110-L110

https://github.com/code-423n4/2021-10-ambire/blob/bc01af4df3f70d1629c4e22a72c19e6a814db70d/contracts/wallet/Zapper.sol#L126-L126

https://github.com/code-423n4/2021-10-ambire/blob/bc01af4df3f70d1629c4e22a72c19e6a814db70d/contracts/wallet/Zapper.sol#L131-L131

Using for (uint i=0; i!=len; i++) {} to control for loops introduces inconsistent code style.

Recommendation

Change from != to < for all for loops.

Ivshti commented 2 years ago

fixed in https://github.com/AmbireTech/adex-protocol-eth/commit/10a88d499e44ff083307fded4631e59f5acef7e1

GalloDaSballo commented 2 years ago

Agree with the finding, using < is more consistent and simpler to understand Sponsor has mitigated