ghoul-sol
commented
3 years ago This is for sure confusing and should be refactored. However, it has very low priority so I'm going to add it to the backlog.
Open code423n4 opened 3 years ago
ghoul-sol
commented
3 years ago This is for sure confusing and should be refactored. However, it has very low priority so I'm going to add it to the backlog.
Handle
gpersoon
Vulnerability details
Impact
There are 3 error enums, which have overlapping values. This allows for mistakes with error codes and might make troubleshooting of deployed code more difficult. I couldn't find any mistakes in the current code; but changes in the future might introduce mistakes.
Proof of Concept
ErrorReporter.sol: contract ComptrollerErrorReporter { enum Error { NO_ERROR, UNAUTHORIZED, COMPTROLLER_MISMATCH,
contract TokenErrorReporter { enum Error { NO_ERROR, UNAUTHORIZED, BAD_INPUT,
CarefulMath.sol contract CarefulMath { enum MathError { NO_ERROR, DIVISION_BY_ZERO,
Tools Used
Editor
Recommended Mitigation Steps
Insert dummy values in the enums to make sure all equivalent numeric values are different. Take care that the same enum values still have the same underlying value to prevent new mistakes. You could for example do the following: ComptrollerErrorReporter NO_ERROR = 0 ComptrollerErrorReporter UNAUTHORIZED = 1 ComptrollerErrorReporter COMPTROLLER_MISMATCH = 2 TokenErrorReporter NO_ERROR = 0 TokenErrorReporter UNAUTHORIZED = 1 TokenErrorReporter BAD_INPUT = 102 CarefulMath.sol NO_ERROR = 0 CarefulMath.sol DIVISION_BY_ZERO = 201 Note: In a few occasions there is a reliance on the fact that NO_ERROR = 0; i'll make a seperate issue for this Note this might break compatibility with Compound and/or other deployed code.