Fallback handlers can trick users into calling functions of the AmbireAccount contract

[code423n4]

Lines of code

https://github.com/AmbireTech/ambire-common/blob/5c54f8005e90ad481df8e34e85718f3d2bfa2ace/contracts/AmbireAccount.sol#L92

Vulnerability details

Fallback handlers can trick users into calling functions of the AmbireAccount contract

Selector clashing can be used to trick users into calling base functions of the wallet.

Impact

Fallback handlers provide extensibility to the Ambire wallet. The main idea here is that functions not present in the wallet implementation are delegated to the fallback handler by using the fallback() function.

Function dispatch in Solidity is done using function selectors. Selectors are represented by the first 4 bytes of the keccak hash of the function signature (name + argument types). It is possible (and not computationally difficult) to find different functions that have the same selector.

This means that a malicious actor can craft a fallback handler with a function signature carefully selected to match one of the functions present in the base AmbireAccount contract, and with an innocent looking implementation. While the fallback implementation may seem harmless, this function when called will actually trigger the function in the base AmbireAccount contract. This can be used, for example, to hide a call to setAddrPrivilege() which could be used to grant control of the wallet to the malicious actor.

This is similar to the exploit reported on proxies in this article, which caused the proposal of the transparent proxy pattern.

As further reference, another similar issue can be found in the DebtDAO contest that could lead to unnoticed calls due to selector clashing (disclaimer: the linked report is authored by me).

Recommendation

It is difficult to provide a recommendation based on the current design of contracts. Any whitelisting or validation around the selector won't work as the main entrypoint of the wallet is the AmbireAccount contract itself. The solution would need to be based on something similar to what was proposed for transparent proxies, which involves segmenting the calls to avoid clashing, but this could cripple the functionality and simplicity of the wallet.

Assessed type

Other

[Ivshti]

I’m not sure if this is applicable: the use case of this is the Ambire team pushing out fallback handlers and allowing users to opt into them. While this does leaves an opportunity for us to be that malicious actor, I’m not sure there’s a better trade off here.

[Picodes]

The scenario is convincing provided the attacker manages to have its malicious implementation of fallbackHandler used by Ambire wallet users, which seems unlikely but doable. Furthermore, as there are no admin roles here, the possibility of this attack by the Ambire team is worth stating.

Overall, I think Medium severity is appropriate. I agree with the previous comments that there is no clear mitigation though aside from warning users about this.

[c4-judge]

Picodes marked the issue as satisfactory