Low Severity Zeppelin Issues

[filcap]

Severity: Low

• [x] 1. Pricer tests do not check for error bounds (pg 11)
• [ ] 2. No error analysis in Formula (pg 11)
• [x] 3. Unsafe math operations in Formula (pg 12)
• [x] 4. Formula priceAtInitialAuction may return zero (pg 12)
• [x] 5. Exchange values in AutonomousAutoConverter are not tested (pg 12)
• [x] 6. Token balance assertions should revert the transaction (pg 13) WONT FIX
• [x] 7. Unused roots mapping in the MTNToken contract (pg 13) WONT FIX
• [x] 8. Subscriptions cannot be cancelled until first withdrawal (pg 13)
• [x] 9. Inconsistent visibility for withdrawal functions (pg 14)
• [x] 10. Duplicated code in multiSubWithdrawFor (pg 14) - -- WONT FIX
• [x] 11. Off-by-one error in Auctions fallback (pg 14)
• [x] 12. Partial refunding is only implemented for returning buyers (pg 14)
• [x] 13. Issues with minimum auction price (pg 15)
• [x] 14. Public multiSubWithdraw function accepts integers instead of addresses (pg 15)
• [X] 15. Monolithic codebase (pg 15) -- WONT FIX
• [X] 16. Use OpenZeppelin and Notes (pg 16) -- WONT FIX

[filcap]

@patidarmanoj10 when these are ready to be worked on lets talk internally to triage

[jgarzik]

Number 15 - monolithic codebase - WONTFIX

[filcap]

Agreed on 15. the truffle-flattener is interesting, but i think right now its too much of a change.

[jgarzik]

WONTFIX for number 16, unless NA wishes to disagree & override my logic (your expertise trumps mine):

I'm a big fan of OpenZeppelin, and I think we used some of their code, but at this late stage, using OpenZeppelin would seem to imply -- if we're Doing It Right -- a whole new raft of audits.

[filcap]

I agree on 16.

[patidarmanoj10]

Number 10 - WONTFIX . multiSubWithdrawFor is using If condition ( instead of require) to skip failed cases and process the transfer of remaining. Hence these 4-5 lines code looks duplicate but its not actually. If we call subWithdrawFor inside multiSubWithdrawFor then one pair fail will always fail for all.

[patidarmanoj10]

Number 8- Removed this require check for code readability only. This has no impact on cancellation because lastWithdrawTime is initialized with startTime

[patidarmanoj10]

Number 7 - WONT FIX because this may be used by validator in import-export.

[patidarmanoj10]

Number 6 - WONT FIX. Other auditor suggested to remove this require check to save gas for all transfer . I agree it will be little bit extra gas for failed transaction. Execution will revert from safe math instead of first line. We need to chose whether we want to save gas for which specific scenario. Number of successful transfer will be more than failed transfer hence would prefer to save gas for success transfer.

[jgarzik]

@patidarmanoj10 Agree we want to save gas for successful transfer > failed transfer

[rokso]

Number 4 is fixed with issue #25 as both were related to same logic, PR is submitted.

[rokso]

Number 1- I have added one test and the error delta is very less. I did spend some time on the formula in Pricer and formula is already optimized and no further scope to optimization.