Submission Details
Severity:
Not checking return value of `add` and `remove` in `CurrencyManager.sol`
Vulnerability details
CurrencyManager::addCurrency function :
function addCurrency(
address currency
) external override onlyRole(ADMIN_ROLE) {
if (currency == address(0))
revert CurrencyManagerError("Cannot be null address");
if (_whitelistedCurrencies.contains(currency))
revert CurrencyManagerError("Already whitelisted");
_whitelistedCurrencies.add(currency);
//@audit return value of add not checked
emit CurrencyWhitelisted(currency);
}
CurrencyManager::removeCurrency function :
function removeCurrency(
address currency
) external override onlyRole(ADMIN_ROLE) {
if (!_whitelistedCurrencies.contains(currency))
revert CurrencyManagerError("Not whitelisted");
_whitelistedCurrencies.remove(currency);
//@audit return value of remove not checked
emit CurrencyRemoved(currency);
}
add and remove are functions from EnumerableSet contract from OpenZeppelin, which returns true in case the operation was successful and false if it was not. But here the return values are not checked.
Impact
If function reverts due to some reason we won't be notified which can lead to silent failures.
Tools used
Manual review
Recommended mitigation
Check the return values of remove and add, incase they fail for some reason it will be easily tracked.
Example :
In addCurrency function
++ bool added = _whitelistedCurrencies.add(currency);
++ if (!added)
++ revert CurrencyManagerError("Failed to add currency to whitelist");
In removeCurrency function
++ bool removed = _whitelistedCurrencies.remove(currency);
++ if (!removed)
++ revert CurrencyManagerError("Failed to remove currency from whitelist");
Prize pool breakdown
Total prize
15,000 USDC
nSLOC
54128 USDC / LOC
14,000 USDC
1,000 USDC
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.