c4-bot-8 commented 7 months ago

Lines of code

https://github.com/Tapioca-DAO/tap-token/blob/20a83b1d2d5577653610a6c3879dff9df4968345/contracts/options/TapiocaOptionBroker.sol#L365-L403

Vulnerability details

Impact

When a user participates in the options system via TapiocaOptionBroker.sol, they mint an otap position in exchange for their tolp position. The time of creation is recorded, and the user is allowed to exercise the option only after a full epoch duration, which is a week.

 if (block.timestamp < oTAPPosition.entry + EPOCH_DURATION) {
        revert OneEpochCooldown();
    }

The variable netDepositedForEpoch records how much liquidity the system expects to hold in any given epoch, and is used to calculate the rewards for the users. On calling participate, this variable is increased for the next epoch, and decreased on the expiration epoch.

netDepositedForEpoch[epoch + 1][lock.sglAssetID] += int256(uint256(lock.ybShares));
netDepositedForEpoch[lastEpoch + 1][lock.sglAssetID] -= int256(uint256(lock.ybShares));

The issue is this accounting is done via epoch, while the cooldown is accounted via block.timestamp. So if a user participates at the very beginning of an epoch, their option will be exercisable after 7 days, which is very close to the time when the epoch number can be incremented. If the user is able to call exerciseOption before the epoch number is incremented, they can even participate in the current epoch! This is because the epoch number has not yet been updated, but the EPOCH_DURATION has already passed.

In this scenario, the user gets access to the rewards of the very same epoch they participated in, which should be impossible. They can also cause the contract to run out of rewards, as they are claiming rewards they shouldnt be owed. The contract calculates rewards based on the current net deposit, which hasnt been updated yet since the epoch number hasnt been updated yet.

uint256 eligibleTapAmount = muldiv(tOLPLockPosition.ybShares, gaugeTotalForEpoch, netAmount);
eligibleTapAmount -= oTAPCalls[_oTAPTokenID][cachedEpoch];

Thus the user can take out rewards based on the wrong netAmount, which can lead to other users not getting their rewards, since the eligibleAmount is decreased by users not owed any rewards.

Proof of Concept

The attack is carried out in the following steps. Lets assume the current epoch has JUST started, and is epoch 1. Let's assume the netAmount for this epoch is 100.

Alice calls participate. The netAmount[2] (next epoch) is increased by her deposit amount of 100.
Alice waits 1 week.
Alice then calls exerciseOption. Lets assume newEpoch() hasnt been called yet.
Alice passes the OneEpochCooldown check. Alice gets assigned rewards. Alice's rewards are = 100 gaugeTotalForEpoch / netAmount[1], which is = 100total/100=total.
The other depositors who hadnt collected their rewards yet cannot collect hem anymore. This is because there are not enough tokens for the rewards anymore. This is because the contract used netAmount[1] as the denominator while there are more than netAmount[1] shareholders collecting rewards.

Since this allows users to collect rewards in the same epoch and break the accounting, and can be timed very effectively, this is a high severity issue.

Tools Used

Manual Review

Recommended Mitigation Steps

The main issue is that the OneEpochCooldown is not measured in epochs and is measured in time instead, which will not necessarily always coincide. Can be fixed in a variety of methods:

Check OneEpochCooldown based on the epoch number, not the timestamp.
When calling exerciseOption, check if the epoch has completed. Similar to what is done in the participate function

if (_timestampToWeek(block.timestamp) > epoch) revert AdvanceEpochFirst();