Serious BUG in the example DAO contract on the official Ethereum.org website?

Question

when I look through the DAO (The Blockchain Congress) example from the official Ethereum website here

https://www.ethereum.org/dao

I noticed something strange, specifically for the addMember and removeMember functions :

/**
 * Add member
 *
 * Make `targetMember` a member named `memberName`
 *
 * @param targetMember ethereum address to be added
 * @param memberName public name for that member
 */
function addMember(address targetMember, string memberName) onlyOwner public {
    uint id = memberId[targetMember];
    if (id == 0) {
        memberId[targetMember] = members.length;
        id = members.length++;
    }

    members[id] = Member({member: targetMember, memberSince: now, name: memberName});
    emit MembershipChanged(targetMember, true);
}

/**
 * Remove member
 *
 * @notice Remove membership from `targetMember`
 *
 * @param targetMember ethereum address to be removed
 */
function removeMember(address targetMember) onlyOwner public {
    require(memberId[targetMember] != 0);

    for (uint i = memberId[targetMember]; i<members.length-1; i++){
        members[i] = members[i+1];
    }
    delete members[members.length-1];
    members.length--;
}

for addMember, it adds a mapping of memberId from address to uint, but when deleting a member, it just basically removes the member from the members array but not changing the memberId mapping, doesn't that mean the whole thing may get messed up after multiple member adding and removing operations?

I mean, if I add address 0x1, 0x2 and 0x3 as the first three members, since there are 2 default members (0x0 and owner), so now I have five members with memberId mapped 0 to 4 like

memberId[0x0] = 0
memberId[owner] = 1
memberId[0x1] = 2
memberId[0x2] = 3
memberId[0x3] => 4 

and the members array basically have addresses like

members[0]={member: 0}
members[1]={member: owner}
members[2]={member: 0x1}
members[3]={member: 0x2}
members[4]={member: 0x3}

Now I remove member 0x2, and the member array ends up with length 4 like

members[0]={member: 0}
members[1]={member: owner}
members[2]={member: 0x1}
members[3]={member: 0x3}

however since the memberId are not updated, it is still

memberId[0x0] = 0
memberId[owner] = 1
memberId[0x1] = 2
memberId[0x2] = 3
memberId[0x3] = 4 

and it seems all hell will break loose if you do further member adding or removing operations. For example if you add another member 0x4, then it will end up with the same memberId 0x3, resulting in something like

members[0]={member: 0}
members[1]={member: owner}
members[2]={member: 0x1}
members[3]={member: 0x3}
members[4]={member: 0x4}

memberId[0x0] = 0
memberId[owner] = 1
memberId[0x1] = 2
memberId[0x2] = 3
memberId[0x3] = 4 
memberId[0x4] = 4

and that's actually the least problematic scenario. Worse is if you decide to call remove 0x2 again, 0x3 will actually be removed, ending up with something like

members[0]={member: 0}
members[1]={member: owner}
members[2]={member: 0x1}

memberId[0x0] = 0
memberId[owner] = 1
memberId[0x1] = 2
memberId[0x2] = 3
memberId[0x3] = 4 

Or if I call add member with 0x2 again, it will actually REPLACE 0x3 instead of adding a new member

members[0]={member: 0}
members[1]={member: owner}
members[2]={member: 0x1}
members[3]={member: 0x2}

memberId[0x0] = 0
memberId[owner] = 1
memberId[0x1] = 2
memberId[0x2] = 3
memberId[0x3] = 4 

Considering the code on the official website should be written by Ethereum experts and code gurus, I'm really not sure what is happening here. Is this really such a serious bug that somehow got past all the code reviewers, or is it intended as a buggy code example? Or have I missed something so that not updating the memberId mapping in the removeMember function is actually okay?

Answer 1

I think the bug can be fixed by adding memberId[members[i].member]--; and memberId[targetMember]=0; to the removeMember function as below:

function removeMember(address targetMember) onlyOwner public {
  require(memberId[targetMember] != 0);

  for (uint i = memberId[targetMember]; i<members.length-1; i++){
    members[i] = members[i+1];
    memberId[members[i].member]--;
  }
  memberId[targetMember] = 0;
  delete members[members.length-1];
  members.length--;
}

(edit: updated to include comment below.)