publisher: oppositeAnnouncement overwrites another opposition at index 0

[gundas]

I think the code at https://github.com/CORIONplatform/solidity/blob/master/publisher.sol#L231-L234 is a mistake and should be deleted - it will always assign foundEmptyArrayID = true and newArrayID = 0 in the first iteration of the loop:

            if ( ! announcements[opponents[msg.sender][a]].open) {
                delete opponents[msg.sender][a];
                if ( ! foundEmptyArrayID ) {
                    foundEmptyArrayID = true;
                    newArrayID = a;
                }
            }
Problem code below -  it is run on  the first iteration of the loop:
            if ( ! foundEmptyArrayID ) {
                foundEmptyArrayID = true;
                newArrayID = a;
            }

So as a result the function will overwrite the first existing announcement opposition for the sender:

        if ( foundEmptyArrayID ) {
            opponents[msg.sender][newArrayID] = id;
        } else {
            opponents[msg.sender].push(id);
        }

On a side note - the method might also leave empty (deleted) array values since there could be more than one closed opposition. Not sure if that is a problem or not - maybe mapping is a more appropriate data structure to store oppositions instead of array?

[iFA88]

I clarified the code, please check again: https://github.com/CORIONplatform/solidity/commit/a1b3dd99e8a53b90281136b9900a13e4ec287928

[gundas]

Negative :) This change while fixes the original problem introduces a new bug - in certain cases it would allow to cast multiple opposition votes from the same account. This is because of the newly added break clause - for example if empty spot (closed announcement) is found earlier in the array than a duplicate vote.

So, the loop needs to be fully executed for the sake of duplicates check. I think the only change need is to remove the https://github.com/CORIONplatform/solidity/blob/master/publisher.sol#L231-L234 lines of code.

[iFA88]

Yes you have right. Please check again: https://github.com/CORIONplatform/solidity/commit/1e65a40d0fb010ad8999c8dc2b0b912062008e1e

[gundas]

Yes, I think now it is fine.

[iFA88]

Thanks!

[gundas]

Just spotted on more thing - you have swapped the balance check and modifying the opponents array - previously (on the main branch) it would first check if the balance is above zero and then put the vote in the opponents array. Now the order opposite.

Technically (due to transaction) it is more or less the same, but the original approach was nicer, IMHO.