Rewards/store: Fix claimed rewards' disappearance

[e18r]

This PR is the product of some awesome team work. @chadoh helped me try to reproduce the issue locally; @stellarmagnet taught me how to inspect the state of a DAO on Rinkeby; @ottodevs generously spent many of his sleeping hours going with me through the steps to publish my
solution to Rinkeby and make it work there; and @topocount debugged an issue with claiming rewards. Thank you all so much!

On Rinkeby, when a reward is claimed it suddenly disappears from My Rewards, and reappears after switching tabs. Since I was unable to reproduce this on my local environment, I investigated the root cause of this problem and found the following facts:

F1. The recently-claimed reward disappears because it gets filtered out from the myRewards object by the app state reducer.

F2. The app state reducer filters out the reward because its userRewardAmount property suddenly becomes zero.

F3. The reward with userRewardAmount set to zero comes from the store's onRewardClaimed function, which gets the reward's properties from getRewardById.

F4. The getRewardById function gets the userRewardAmount property from the rewardAmount return value of the contract's getReward function.

F5. The rewardAmount return value of the contract's getReward function is calculated dynamically by the contract's _calculateRewardAmount function.

F6. rewardAmount is the reward's amount multiplied by the user's balance at disbursement date, divided by the reference token's supply at disbursement date.

F7. In order for rewardAmount to be zero, either the reward's amount, the user's balance or the token's supply have to be zero.

After determining this, I formulated the following hypothesis:

H1. rewardAmount is zero because the user's balance is zero, because _calculateRewardAmount is receiving the wrong user; a user who doesn't possess any of that token. This happens when the store's getRewardById function doesn't receive a userAddress param.

My hypothesis H1 is supported by the following facts:

F8. On the UI, the reward reappears whenever tabs are switched. What happens underneath is that the store's onRefreshRewards function gets called. This function calls getRewardById just like onRewardClaimed (see F3), but unlike it, it specifies a user address.

F9. Another store function, onRewardAdded, also calls getRewardById without specifying a user address, and the same behaviour on the UI can be observed as in onRewardClaimed: the
reward's rewardAmount field is set to zero. This is less noticeable, but can be observed by creating a reward while on the My Rewards tab.

F10. The userRewardAmount property of the recently-claimed reward is not the only property that becomes zero. The same happens to timeClaimed. On the contract, timeClaimed is a map from addresses to integers. The contract's getReward function uses the calling user's address to get the value. If the user's address didn't exist on the map, that could explain why it ends up being zero.

F11. @topocount solved a similar bug a while ago. The description he gave me of the bug matched perfectly H1.

In order to test my hypothesis, @ottodevs and I published my locally modified version of the Rewards app to Rinkeby, created a Rinkeby DAO and installed it there. The bug went away. You can test it here.

So I can say with confidence that this PR fixes #1518 :)

The only remaining mistery is why can't this be reproduced locally. I don't know enough about the differences between Ganache and Rinkeby (or between the local environment and other blockchain
environments in general) in order to answer that question. But I do believe it's a relevant question that needs further inquiry in order to determine if the best path forward with similar issues is to
somehow make the local environment more closely resemble the blockchain, or to acknowledge the need to sometimes deploy custom Rinkeby DAOs, just like in this case.

Finally, it's important to note that this PR modifies slightly the audited contract code. That's because this is the best way to solve the bug at hand. But if modifying the contract code is not a possibility right now, I can think of some inelegant workarounds.

[ottodevs]

Just some checks about using tx.origin once we confirm its use does not represent any security concern, then I will fine approving this :)

Did you experience issues with msg.sender because the proxy?

Amazing work and explanation about the process @e18r !

[ottodevs]

I don't think keeping the comments would hurt, but on the other hand the events names seem quite intuitive, so I am "ok" with the change.

[e18r]

Yeah, a bit redundant and unnecessary. But if they're part of some convention or standard practice, it might make sense to leave them.

[ottodevs]

I am not really sure if this should be msg.sender instead... I invoke @Quazia for this or try to read this to ensure tx.origin is what we need: https://ethereum.stackexchange.com/questions/76929/tx-origin-alternative

[e18r]

@ottodevs thank you for pointing this out. I decided to use tx.origin because in some cases the proxy address was showing up instead of the user's. But I'd need to test this more. Also, I'm gonna read about the security implications of tx.origin, which I was totally unaware of: ethereum/solidity#683

[e18r]

Yes. After reading about it, tx.origin is insecure and not future-proof. I'm going to test if msg.sender works as expected and let you know.

[Quazia]

Yeah, neither one of these will behave as we want.

[e18r]

@Quazia after testing msg.sender it does work as expected for the claim transaction event. Or am I missing something?

[ottodevs]

Same here: I am not really sure if this should be msg.sender instead... I invoke @Quazia for this or try to read this to ensure tx.origin is what we need: https://ethereum.stackexchange.com/questions/76929/tx-origin-alternative

[e18r]

@ottodevs I changed tx.origin to msg.sender for the case of claiming rewards, and I reverted the changes for the case of creating rewards, because msg.sender in that case issues the proxy address.

This means that the bug will still be present for the case of creating rewards, but in practice it's barely noticeable at all. It will only manifest itself if all of the following is true:

a) A user creates a reward while on the "My Rewards" tab
b) The created reward can be claimed by the user
c) The user can create rewards directly without going through a vote

This is a pretty rare situation and it isn't covered on the issue this PR fixes, so I consider it safe to leave it for an eventual future PR.

[chadoh]

👏 👏 👏

Amazing investigative and communicative work! The commit messages and PR description are works of art.

I'm still flummoxed that we're seeing different behavior in Rinkeby than what we see locally, and I am curious if @topocount or @Quazia have any ideas why that might be. I'd like us to understand that, before we move on, given that this mismatch implies that we may have similar bugs lurking around that we can't catch during development.

Also, I want to note that the root cause is a difference between the arguments a function expects to receive and what callers of that function actually passed, and I want to further note that this whole class of error goes away with a fully-implemented type system!

[ottodevs]

LGTM