search

Safemoon-Protocol / safemoon.sol

MIT License
79 stars 98 forks source link

certik fixes #6

Closed dvdknaap closed 1 year ago

dvdknaap commented 3 years ago

This PR fixes the following fixes from the certik audit report: SSL-01 | Incorrect error message SSL-02 | Redundant code SSL-05 | Variable could be declared as constant SSL-09 | Function and variable naming doesn't match the operating environment [partial] SSL-11 | Typos in the contract

Additions: code formatting small improvement in for loop

These updates should be save to change and shouldn't break the contract.

edit: added summary of the updates

Jovonni commented 3 years ago

You didn’t actually fix any of the Certik recommendations. This looks like you breaking out some statements into there own variables and changing code layout (white spaces, indentations, etc)

dvdknaap commented 3 years ago

You didn’t actually fix any of the Certik recommendations. This looks like you breaking out some statements into there own variables and changing code layout (white spaces, indentations, etc)

I've read their recommendations so maybe you have to read them to. This is what i did, these are most of the recommendations like i said

niccolopetti commented 3 years ago

Actually @Jovonni is right, this PR doesn't fix any of the real problems existing in the code and addressed by certik's audit, it fixes a bunch of typos, renames some functions/variables and makes some variables constant. In conclusion, the name of the PR "certik fixes" is a bit strong and misleading from the actual content of the PR.

dvdknaap commented 3 years ago

Actually @Jovonni is right, this PR doesn't fix any of the real problems existing in the code and addressed by certik's audit, it fixes a bunch of typos, renames some functions/variables and makes some variables constant. In conclusion, the name of the PR "certik fixes" is a bit strong and misleading from the actual content of the PR.

Not really i've added a summary so you can see what is changes. the changes that are done are save and shouldn't harm the contract.

niccolopetti commented 3 years ago

I saw what was changed as I read the code, and surely adding that summary is nice, declaring "solved most of the certik fixes" I guess people would expect to see the major problems solved, i.e the ones that the devs can use to scam all the token holders, that is not the case with this PR and the summary clarifies it. I agree these updates won't break the contract, but the overral behavior of the contract is unchanged. Ps: don't take comments on a personal level, we are just giving a feedback on your PR and there should be nothing to get mad at🙂

Jovonni commented 3 years ago

My thoughts exactly @niccolopetti 🚀

james3756 commented 3 years ago

J'ai vu ce qui a été changé en lisant le code, et l'ajout de ce résumé est sûrement agréable, déclarant "résolu la plupart des correctifs de certik" Je suppose que les gens s'attendent à voir les problèmes majeurs résolus, c'est-à-dire ceux que les développeurs peuvent utiliser pour arnaquer tous les détenteurs de jetons, ce n'est pas le cas avec ce PR et le résumé le clarifie. Je suis d'accord que ces mises à jour ne rompront pas le contrat, mais le comportement excessif du contrat reste inchangé. Ps: ne prenez pas de commentaires à un niveau personnel, nous donnons juste un retour sur votre PR et il ne devrait y avoir rien contre quoi se fâcher🙂

hello can you contact me on discord I have some question if possible my discord is Alekesi#8223