code423n4 commented 2 years ago

Handle

ye0lde

Vulnerability details

Impact

The input parameter _amount is not checked for 0 in functions deposit or withdraw.
While both functions will eventually revert not before calling updatePool and executing quite a bit of code using a parameter that does not make sense.

Depositing or withdrawing 0 tokens does not seem to be a necessary use case to support.

Proof of Concept

In the case of withdraw here: https://github.com/code-423n4/2022-01-trader-joe/blob/a1579f6453bc4bf9fb0db9c627beaa41135438ed/contracts/RocketJoeStaking.sol#L116-L135

This line at 117 allows any user to proceed providing valid user information even though the values are zero:

UserInfo storage user = userInfo[msg.sender];

And the require at 118 basically checks that 0 >= 0:

require(
            user.amount >= _amount,
            "RocketJoeStaking: withdraw amount exceeds balance"
        );

The deposit function is similar: https://github.com/code-423n4/2022-01-trader-joe/blob/a1579f6453bc4bf9fb0db9c627beaa41135438ed/contracts/RocketJoeStaking.sol#L96-L112

Tools Used

Visual Studio Code, Remix

Recommended Mitigation Steps

I would recommend adding:

require(_amount != 0, "error message")

at the start of each function.

cryptofish7 commented 2 years ago

withdraw(0) and deposit(0) is how people will claim their rewards.

dmvt commented 2 years ago

Invalid. The recommended remediation would break existing functionality.

code-423n4 / 2022-01-trader-joe-findings