search

code-423n4 / 2022-05-velodrome-findings

0 stars 0 forks source link

Wrong calculation for the new `rewardRate[token]` can cause some of the late users can not get their rewards #186

Open code423n4 opened 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L597-L612

Vulnerability details

uint bribeStart = block.timestamp - (block.timestamp % (7 days)) + BRIBE_LAG;
uint adjustedTstamp = block.timestamp < bribeStart ? bribeStart : bribeStart + 7 days;
if (rewardRate[token] == 0) _writeRewardPerTokenCheckpoint(token, 0, adjustedTstamp);
(rewardPerTokenStored[token], lastUpdateTime[token]) = _updateRewardPerToken(token);
_claimFees();

if (block.timestamp >= periodFinish[token]) {
    _safeTransferFrom(token, msg.sender, address(this), amount);
    rewardRate[token] = amount / DURATION;
} else {
    uint _remaining = periodFinish[token] - block.timestamp;
    uint _left = _remaining * rewardRate[token];
    require(amount > _left);
    _safeTransferFrom(token, msg.sender, address(this), amount);
    rewardRate[token] = (amount + _left) / DURATION;
}

In Gauge.sol#notifyRewardAmount(), the updated rewardRate for the token: rewardRate[token] is calculated based on the newly added amount of tokens (amount), the remaining amount of existing rewards (_left), and the DURATION.

While the DURATION is 5 days, the period from the current time to periodFinish[token] is much longer.

rewardPerToken() is calculated based on the current time, lastUpdateTime[token], and rewardRate[token].

https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L375-L380

function rewardPerToken(address token) public view returns (uint) {
    if (derivedSupply == 0) {
        return rewardPerTokenStored[token];
    }
    return rewardPerTokenStored[token] + ((lastTimeRewardApplicable(token) - Math.min(lastUpdateTime[token], periodFinish[token])) * rewardRate[token] * PRECISION / derivedSupply);
}

lastUpdateTime[token] will frequently be updated to the current timestamp by _updateRewardForAllTokens().

See: https://github.com/code-423n4/2022-05-velodrome/blob/7fda97c570b758bbfa7dd6724a336c43d4041740/contracts/contracts/Gauge.sol#L460-L469

As a result, rewardPerToken() can be much higher than expected, which makes the total amount of reward tokens less than the total amount of rewards accumulated by all the users.

This makes the users who claim the rewards later unable to retrieve their rewards as the balance can be insufficient.

PoC

  1. Alice and Bob both deposited 1,000 stake token to Gauge at 1653091200 (May 21 2022 00:00:00 GMT+0000)

  2. Admin called notifyRewardAmount() add 1,000 DAI at 1653100000 (May 21 2022 02:26:40 GMT+0000)

  1. Alice withdrawn and getReward() at 1654041800 Jun 01 2022 00:03:20 GMT+0000, get ~1,000 DAI
  1. Bob tried to getReward(), the transaction will revert due to insufficient balance.

Recommendation

Consider calculating rewardRate base on timeUntilNextPeriodFinish to next period finish:

uint nextPeriodFinish = adjustedTstamp + DURATION;
uint timeUntilNextPeriodFinish = nextPeriodFinish - block.timestamp;
if (block.timestamp >= periodFinish[token]) {
    _safeTransferFrom(token, msg.sender, address(this), amount);
    rewardRate[token] = amount / timeUntilNextPeriodFinish;
} else {
    uint _remaining = periodFinish[token] - block.timestamp;
    uint _left = _remaining * rewardRate[token];
    require(amount > _left);
    _safeTransferFrom(token, msg.sender, address(this), amount);
    rewardRate[token] = (amount + _left) / timeUntilNextPeriodFinish;
}
require(rewardRate[token] > 0);
uint balance = IERC20(token).balanceOf(address(this));
require(rewardRate[token] <= balance / timeUntilNextPeriodFinish, "Provided reward too high");
periodFinish[token] = nextPeriodFinish;
pooltypes commented 2 years ago

Duplicate of #141

GalloDaSballo commented 2 years ago

The warden has shown how a desynch between periodFinish and DURATION can cause rewards to be distributed faster than a reward period, leaving the last claimers unable to claim.

While the loss in this scenario, to those claimers can be viewed as total, in reality those people will be able to claim on the next rewards round (unless no more tokens will be minted to that gauge, which is highly unlikely but possible).

For that reason, I believe Medium Severity to be more appropriate, as this will be a temporary loss of yield which for some people may be permanent