PatrickAlphaC / defi-stake-yield-brownie-freecode

52 stars 62 forks source link

Stakers array could contain duplicate entries. #8

Open tonisives opened 2 years ago

tonisives commented 2 years ago

If we have set

 uniqueTokensStaked[_user] = uniqueTokensStaked[_user] + 1;

before, and user calls stakeTokens again with the same token, the

if (uniqueTokensStaked[msg.sender] == 1){
    stakers.push(msg.sender);
}

uniqueTokensStaked for the user will be 1 because it was set in the first staking request. Now msg.sender will be pushed to the stakers array again as a duplicate.

PS: I have not tested this but it seems like an issue.

https://github.com/PatrickAlphaC/defi-stake-yield-brownie-freecode/blob/0dd62b0be06a7c580a6da047ec018a302b20dd7b/contracts/TokenFarm.sol#L94

Solution would be for the updateUniqueTokensStaked function to return a boolean when the mapping value is actually set.

mehdi077 commented 2 years ago

i think the code should be somthing like this. error 2

mehdi077 commented 2 years ago

a cleaner version of the code :) error 2-2

tonisives commented 2 years ago

That seems like one possible solution @mehdi077. Me myself I added a boolean return to the updateUniqueTokensStaked method here.

    function updateUniqueTokensStaked(address _user, address _token)
        internal
        returns (bool)
    {
        if (stakingBalance[_token][_user] <= 0) {
            uniqueTokensStaked[_user] = uniqueTokensStaked[_user] + 1;
            return true;
        }
        return false;
    }

I check this boolean before pushing to the stakers array here

I'm not sure which is the better way. Maybe both are ok. I think for your solution @mehdi077, there might be extra gas cost by looking up the initial_amount. But I'm not sure how gas costs are calculated really.

PatrickAlphaC commented 2 years ago

I think you're right!