code-423n4 / 2021-11-badgerzaps-findings

0 stars 0 forks source link

Missing error messages in require statements #38

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Handle

WatchPug

Vulnerability details

https://github.com/Badger-Finance/badger-ibbtc-utility-zaps/blob/a5c71b72222d84b6414ca0339ed1761dc79fe56e/contracts/SettToRenIbbtcZap.sol#L167-L171

require(_sett != address(0));
require(_token != address(0));
require(
    _withdrawToken == address(WBTC) || _withdrawToken == address(RENBTC)
);

https://github.com/Badger-Finance/badger-ibbtc-utility-zaps/blob/a5c71b72222d84b6414ca0339ed1761dc79fe56e/contracts/SettToRenIbbtcZap.sol#L189-L193

require(_sett != address(0));
require(_token != address(0));
require(
    _withdrawToken == address(WBTC) || _withdrawToken == address(RENBTC)
);

https://github.com/Badger-Finance/badger-ibbtc-utility-zaps/blob/6f700995129182fec81b772f97abab9977b46026/contracts/IbbtcVaultZap.sol#L54-L55

require(_guardian != address(0)); // dev: 0 address
require(_governance != address(0)); // dev: 0 address
GalloDaSballo commented 2 years ago

Disagree with the finding, a missing revert message is not necessary, additionally using //dev comments allows to get the same result when debugging

Lastly, when running a stacktrace you can always track back to the revert condition, the message is not useful for devs, it's higher gas cost and most of the time the end user doesn't care / is shown something else anyway

0xleastwood commented 2 years ago

Agree with sponsor, it makes sense to not add revert messages for functions that only callable for restricted accounts.