MyCut

Summary

The ERC20 token function ERC20::transfer returns a boolean value (true if the transfer was successful). However, the idea of this return values is to revert if the transaction fails. In this protocol the return values are ignored. This problem is anywhere in the protocol where transfer is used. This includes Pot::_transferReward and the ERC20::transferFrom call in ContestManager.sol. This can lead to loss of funds for the user. The ERC20 implementation only reverts on 0-addresses.

Vulnerability Details

transfer() will return true if the transfer was successful and falseif the transfer failed.

A user calls for example Pot::claimCut.

1. The reward of this user is > 0 → The function will be executed.

2. The states of playersToRewards[player], remainingRewards and claimants[] are set

3. _transferReward is called, transfer fails and returns false which is not checked

4. The user has no longer access to his rewards by calling claimCutagain.

Those rewards will be stuck in the contest due to the updated storage variable remainingRewards.

Impact

If a transaction fails somehow, the states are already set. This can lead to loss of funds or unexpected behavior. Since the states are already set, calling this function again cannot undo this problem.

Tools Used

Manual Review

Recommendations

Checking the return values of transfer. Here is an example:

Another popular way to accomplish checked transfer values is by using the SafeERC20 library.