code423n4 commented 1 year ago

Lines of code

https://github.com/AmbireTech/ambire-common/blob/v2/contracts/AmbireAccount.sol#L249

Vulnerability details

[adriro-NEW-M-02]: Wallet design prevents EIP-165 extensibility

The current wallet fallback design prevents the extensibility of the EIP-165 functionality.

Impact

Ambire wallet extensibility is provided by a fallback mechanism. If a fallback handler is defined in slot FALLBACK_HANDLER_SLOT, any non-matching function will be delegated to this address.

This is a simple and elegant pattern that can provide extensibility by forwarding calls to a configured external contract. If the function selector isn't present in the AmbireAccount contract, the call is forwarded to the fallback contract using delegatecall.

There is a drawback to this pattern though. In this design, functions that are both defined in the wallet contract and the fallback handler will be caught by the main contract (AmbireAccount) instead of being delegated to the fallback, since function dispatch happens first in the main contract. The fallback handler won't be able to extend or override functions defined in the wallet contract.

This means that EIP-165 functionality cannot be extended. The implementation of EIP-165 is done by defining a supportsInterface() function that can be queried for interface support. This function is currently defined in the AmbireAccount contract to signal support for ERC721TokenReceiver and ERC1155TokenReceiver. The fallback handler won't be able to extend this functionality as the call can't reach the fallback, it will be caught by the wallet contract and will only answer for the already defined cases.

Any unimplemented or future standard that requires publishing interface support via EIP-165 won't be able to be integrated in the Ambire wallet.

Recommendation

Similar to how ERC165Storage works in the OpenZeppelin library, it is possible to define a mapping that will hold supported interfaces. The fallback (or any transaction to the wallet) can register interfaces by using a function only callable from the same contract.

mapping(bytes4 => bool) private _supportedInterfaces;

function setInterface(bytes4 interfaceId, bool value) external {
  require(msg.sender == address(this), 'ONLY_IDENTITY_CAN_CALL');
  _supportedInterfaces[interfaceId] = value;
}

function supportsInterface(bytes4 interfaceID) external pure returns (bool) {
  return
  interfaceID == 0x01ffc9a7 || // ERC-165 support (i.e. `bytes4(keccak256('supportsInterface(bytes4)'))`).
  interfaceID == 0x150b7a02 || // ERC721TokenReceiver
  interfaceID == 0x4e2312e0 || // ERC-1155 `ERC1155TokenReceiver` support (i.e. `bytes4(keccak256("onERC1155Received(address,address,uint256,uint256,bytes)")) ^ bytes4(keccak256("onERC1155BatchReceived(address,address,uint256[],uint256[],bytes)"))`).
  _supportedInterfaces[interfaceId];
}

Assessed type

Other

Ivshti commented 1 year ago

solved here https://github.com/AmbireTech/ambire-common/commit/bb6d453ac4aa4320c5ba24a2faa81ff56ca4be7c

c4-judge commented 1 year ago

Picodes marked the issue as nullified

code-423n4 / 2023-06-ambire-mitigation-findings