DexLyn Disclosed Report

The add_role() function is used to add a role for a member in the ACL. If the member already exists in acl.permissions, the protocol calculates the permissions as follows:

*perms = *perms | (1 << role)

    public fun add_role(acl: &mut ACL, member: address, role: u8) {
        assert!(role < 128, error::invalid_argument(EROLE_NUMBER_TOO_LARGE));
        if (table::contains(&acl.permissions, member)) {
            let perms = table::borrow_mut(&mut acl.permissions, member);
            *perms = *perms | (1 << role);
        } else {
            table::add(&mut acl.permissions, member, 1 << role);
        }
    }

When revoking a role for a member in the ACL, the protocol currently calculates the permissions as:

*perms = *perms - (1 << role)

    public fun remove_role(acl: &mut ACL, member: address, role: u8) {
        assert!(role < 128, error::invalid_argument(EROLE_NUMBER_TOO_LARGE));
        if (table::contains(&acl.permissions, member)) {
            let perms = table::borrow_mut(&mut acl.permissions, member);
            *perms = *perms - (1 << role);
        }
    }

However, this calculation is incorrect. It should be:

*perms = *perms ^ (1 << role)

Otherwise, if the protocol tries to revoke a role that the user does not actually have, it may end up incorrectly granting them a different role instead. This could cause unexpected permission changes and potentially lead to severe security issues.

1. Suppose a user only has role = 1, so the value of perms is 1 << 1, which equals 2.
2. The operator calls the remove_role() function to remove role = 0 (but the user does not actually have this role). In this case, 1 << role equals 1 << 0, which is 1.
3. The function executes *perms = 2 - 1, resulting in 1.

This means that although the intention was to remove a role the user never had, the calculation mistakenly grants the user a different role instead.