Array index bug in revoke function of GroupAdmin

[nemozqqz]

Description:

In GroupAdmin.sol, when owner revokes oldAdmins from the admins array，each address in oldAdmins is compared with address in admins. While in the for loop in line 35, the index of admins array, uint j is not updated because of the typo ++i, and it should be ++j.

    function revoke(address[] oldAdmins) public onlyOwner{
        for(uint i = 0; i < oldAdmins.length; i++){
            for (uint j = 0; j < admins.length; ++i) { // typo here!
                if (admins[j] == oldAdmins[i]) {
                    admins[j] = admins[admins.length - 1];
                    admins.length--;
                    emit AdminRevoked(oldAdmins[i]);
                    break;
                }
            }
        }
}

Impact:

This bug will make the call of revoke fail and some oldAdmins won't be revoked. Without ++j, j will always be zero. if oldAdmins[0] != admins[0] , the for loop in line 35 won't break, thus variable i will eventually execeed the length of oldAdmins, cause array out-of-bound and finally revert the evm state.
revoke function was added since commit 8e94db42

Fix:

for (uint j = 0; j < admins.length; ++j)

[vietlq]

Also another tip: Instead of i & j, write better names like oldAdmIdx and newAdmIdx. Much harder to confuse old/new compared to i/j.