Submission Details
Severity:
Access Control Omission in `Fallback` Function of `ModuleManager` Contract
## Summary
The The [fallback](https://github.com/Cyfrin/2024-07-biconomy/blob/9590f25cd63f7ad2c54feb618036984774f3879d/contracts/base/ModuleManager.sol#L72) function access control, potentially allowing unauthorized execution of sensitive operations by third-party fallback handlers.
## Vulnerability Details
he fallback function does not enforce access control checks before invoking the fallback handler. This omission assumes that third-party developers will include necessary access control within their handlers, which may not always be the case. This can lead to unauthorized execution of sensitive operations, such as token transfers, if the fallback handler lacks proper access control.
```javascript
fallback() external payable override(Receiver) receiverFallback {
FallbackHandler storage $fallbackHandler = _getAccountStorage().fallbacks[msg.sig];
address handler = $fallbackHandler.handler;
CallType calltype = $fallbackHandler.calltype;
require(handler != address(0), MissingFallbackHandler(msg.sig));
if (calltype == CALLTYPE_STATIC) {
assembly {
calldatacopy(0, 0, calldatasize())
// The msg.sender address is shifted to the left by 12 bytes to remove the padding
// Then the address without padding is stored right after the calldata
mstore(calldatasize(), shl(96, caller()))
if iszero(staticcall(gas(), handler, 0, add(calldatasize(), 20), 0, 0)) {
returndatacopy(0, 0, returndatasize())
revert(0, returndatasize())
}
returndatacopy(0, 0, returndatasize())
return(0, returndatasize())
}
}
if (calltype == CALLTYPE_SINGLE) {
assembly {
calldatacopy(0, 0, calldatasize())
// The msg.sender address is shifted to the left by 12 bytes to remove the padding
// Then the address without padding is stored right after the calldata
mstore(calldatasize(), shl(96, caller()))
if iszero(call(gas(), handler, 0, 0, add(calldatasize(), 20), 0, 0)) {
returndatacopy(0, 0, returndatasize())
revert(0, returndatasize())
}
returndatacopy(0, 0, returndatasize())
return(0, returndatasize())
}
}
}
```
## Impact
The lack of access control in the fallback function of the ModuleManager contract allows unauthorized users to invoke fallback handlers, potentially executing sensitive operations. This can lead to unauthorized token transfers, resulting in financial losses. Additionally, other sensitive operations within fallback handlers can be exploited, creating broader security vulnerabilities. Relying on third-party developers to implement access control introduces inconsistency and increases the risk of exploitation. Overall, this omission significantly undermines the contract's security and integrity.
## Tools Used
Manual Review
## Recommendations
it is recommended to include strict access control measures in fallback handlers,
considering they may contain token transfer logic or other sensitive operations, to avert potential security risks.
Updates
Lead Judging Commences
0xnevi 11 months ago
Submission Judgement Published Assigned finding tags:
finding-fallback-missing-access-control-module-manager
There is indeed no access control within `fallback()` function which violates ERC7579 spec but the impact shown by all issues is insufficient. Need a better impact description/PoC that exceeds violation of ERC7579 to raise the severity of this issue. There will likely be no exploit for staticcall types, given there is not [state change/funds transfer allowed](https://www.rareskills.io/post/solidity-staticcall), so the possible vulnerability would be in the `CALLTYPE_SINGLE`. If no sufficient proof is provided to show a possible exploit, I will likely invalidate these issues.
Prize pool breakdown
Total prize
30,000 USDC
nSLOC
1,43321 USDC / LOC
25,000 USDC
2,750 USDC
2,250 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.