Submitted by kirk-baird, also found by 0v3rf10w and static
Note: this attack requires rJoe to relinquish control during tranfer() which under the current RocketJoeToken it does not. Thus this vulnerability is raised as medium rather than high. Although its not exploitable currently, it is a highly risky code pattern that should be avoided.
This vulnerability would allow the entire rJoe balance to be drained from the contract.
The function deposit() would be vulnerable to reentrancy if rJoe relinquished control flow.
The following lines show the reward calculations in variable pending. These calculations use two state variables user.amount and user.rewardDebt. Each of these are updated after _safeRJoeTransfer().
Thus if an attacker was able to get control flow during the rJoe::tranfer() function they would be able to reenter deposit() and the value calculated for pendingwould be the same as the previous iteration hence they would again be transferred pending rJoe tokens. During the rJoe transfer the would again gain control of the execution and call deposit() again. The process could be repeated until the entire rJoe balance of the contract has been transferred to the attacker.
There are two possible mitigations. First is to use the openzeppelin reentrancy guard over the deposit() function which will prevent multiple deposits being made simultaneously.
The second mitigation is to follow the checks-effects-interactions pattern. This would involve updating all state variables before making any external calls.
cryptofish7 (Trader Joe) confirmed, but disagreed with severity and commented:
LSDan (judge) commented:
