Zaros Part 1
Zaros
DeFiFoundry
60,000 USDC
View results
Previous Next
Submission Details
Severity: medium
Invalid

Lack of Zero Address Check in `setForwarder` Function

rhaydden

Summary

setForwarder function allows the owner to update the forwarder address without validating the new address. This could potentially lead to the forwarder being set to the zero address (0x0), which in turn would render the onlyForwarder modifier ineffective.

Vulnerability Details

Look at this part of the code: https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/external/chainlink/keepers/BaseKeeper.sol#L39-L42

function setForwarder(address forwarder) external onlyOwner {
BaseKeeperStorage storage self = _getBaseKeeperStorage();
self.forwarder = forwarder;
}

This function allows the owner to set the forwarder address to any value, including the zero address. If the forwarder is set to the zero address, it would break the functionality of the onlyForwarder modifier:

modifier onlyForwarder() {
BaseKeeperStorage storage self = _getBaseKeeperStorage();
bool isSenderForwarder = msg.sender == self.forwarder;
​
if (!isSenderForwarder) {
revert Errors.Unauthorized(msg.sender);
}
_;
}

Albeit, with a zero address forwarder, no valid address would be able to pass the check in this modifier effectively disabling any functions that rely on it for access control.

Impact

Likelihood- low, as it requires an error by the contract owner.
Impact- High, as it could significantly disrupt the contract's intended functionality and could cause a DOS too.

All functions using the onlyForwarder modifier would become inaccessible, as no address would be able to pass the authorization check causing a DOS.
This may require a contract upgrade to fix.

Tools Used

Manual code review

Recommendations

Implement a zero address check in the setForwarder function:

function setForwarder(address forwarder) external onlyOwner {
+ if (forwarder == address(0)) {
+ revert Errors.ZeroInput("forwarder");
+ }
BaseKeeperStorage storage self = _getBaseKeeperStorage();
self.forwarder = forwarder;
}
Updates

Lead Judging Commences

inallhonesty Lead Judge
about 1 year ago
inallhonesty Lead Judge about 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.