3

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?

  • 1
    I think you're overestimating how much effort was put into these examples. They're just simple examples of what could be done, there's no audit or anything and no claim is made to their usability in production. In fact, this page is an open source effort, feel free to submit a PR to github.com/ethereum/ethereum-org/edit/master/views/content/… – Tjaden Hess Aug 9 '18 at 16:31
  • 1
    oh well since these examples are displayed on the official website and first-time running the official Mist browser I thought they'd be more careful in writing the sample code. after all, for newbies like me, we may take them as some reference coding templates for making our own entry into the smart contract coding scene. But then yeah if they are indeed such glaring logic errors in the sample smart contracts on the official site I guess it's best to help fix them ASAP. I just find it a bit strange that no one else noticed these bugs sooner for such a popular site in the cryptocurrency scene. – hellopeach Aug 9 '18 at 18:30
  • OP: file a bug. I didn’t study it closely, but it looks like a bug to me. That should 100% be fixed. Better yet, fix it yourself and post a PR. – Thomas Jay Rush Aug 17 '18 at 0:36
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.)

  • 1
    actually, you need to add a memberId[targetMember] = 0, however I'm not sure how to submit the fix to the Ethereum.org to make the change visible on the website – hellopeach Aug 16 '18 at 3:45
  • Good point about memberId. I've added that to the code above... As far as contacting Ethereum.org, I think there are at least two options: We could submit an issue here. Alternately we could clone the repository, make changes to the contract, then submit the changes in a pull request. Feel free to send me a message if you want help or want me to do it. – sfmiller940 Aug 16 '18 at 21:14
  • You should definitely fix that or at least report until they understand because it shouldn’t be there. – Thomas Jay Rush Aug 17 '18 at 0:37
  • It seems to have been fixed recently on github, but not on the website. Maybe there's a delay in syncing the two. – sfmiller940 Aug 17 '18 at 18:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.