Description

The Authenticator interface implements a ConfirmExecution handler, which is executed by the post-handler. This can be used to enforce transaction rules or validate any posttransaction changes. The ConfirmExecution handler can reject a transaction if any rules are violated.

The AllOfAuthenticator and AnyOfAuthenticator authenticators allow use of multiple authenticators at once. Ideally, it should invoke the ConfirmExecution handler for all SubAuthenticators.

func (aoa AllOfAuthenticator) ConfirmExecution(ctx sdk.Context, account sdk.AccAddress, msg sdk.Msg, authenticationData iface.AuthenticatorData) iface.ConfirmationResult {
	for _, auth := range aoa.executedAuths {
		if confirmation := auth.ConfirmExecution(ctx, nil, msg, authenticationData); confirmation.IsBlock() {
			return confirmation
		}
	}
	return iface.Confirm()
}

The ConfirmExecution handler for these authenticators iterate over executedAuths and invoke ConfirmExecution for the SubAuthenticators. However, executedAuths is never actually set anywhere.

Impact

The ConfirmExecution handlers for the SubAuthenticators are never invoked. Therefore, the postexecution checks are always skipped. This could allow an attacker to bypass any posttransaction checks and/or violate spending and transaction limits.

Recommendations

The ConfirmExecution handler for the AllOfAuthenticator and AnyOfAuthenticator should always invoke the ConfirmExecution handlers for all registered SubAuthenticators.

Remediation

The issue was fixed in PR #6787↗ with removal of executedAuths. However, the implementation was updated to allow ConfirmExecution to succeed if the ConfirmExecution handler for any of the SubAuthenticators pass.

Upon Discussion with the team, this was reverted to the original implementation in PR #6838↗ and merged in commit d56de736↗. This now requires ConfirmExecution handlers for all of the SubAuthenticators to succeed.