Biconomy: Nexus
Biconomy
HardhatFoundry
30,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

`ModuleManager#fallback()` is not correctly implemented

bauchibred

Summary

ModuleManager#fallback() is not correctly implemented since it doesn't take the payable key word to factor.

Vulnerability Details

Take a look at https://github.com/Cyfrin/2024-07-biconomy/blob/9590f25cd63f7ad2c54feb618036984774f3879d/contracts/base/ModuleManager.sol#L72-L110

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())
}
}
}

This is the fallback function and it's used to manage incoming calls using designated handlers based on the call type.

Issue however is that in both call types, the msg.value received/attached is never taken into account when making the calls, which then causes for the functionality to not be able to correctly function when there is a msg.value attached.

Impact

Core functionality is broken, considering the functionality is expected to have msg.value attached but in it's execution of both types this msg.value is not queried/used which breaks the functionality.

Tools Used

Manual review

Recommendations

Since the fallback is marked as payable consider attaching this msg.value to the call types.

Updates

Lead Judging Commences

0xnevi Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

finding-cannot-msg.value-not-forwarded

Support

FAQs

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