TempleGold
TempleDAO
Foundry
25,000 USDC
View results
Previous Next
Submission Details
Severity: low
Valid

Unexpected step-wise changes in the release schedule of templeGold minting rate when updating the `vestingFactor`

jacopod

Summary

When the vestingRate is updated, it not only affects the vesting rate of future tokens, but also of past released tokens that haven't been minted yet, i.e., tokens since last call to mint().

Vulnerability Details

The internal function _getMintAmount() controls the exact amount of templeGold tokens that can be minted at any given time. Tokens are released following a linear vesting formula: at any given time, the amount to be minted is proportional to the time since the last mint (block.timestamp - _lastMintTimestamp) and the minting rate, (stored in the vestingFactor storage variable):

function _getMintAmount(VestingFactor memory vestingFactorCache) private view returns (uint256 mintAmount) {
uint32 _lastMintTimestamp = lastMintTimestamp;
uint256 totalSupplyCache = _totalDistributed;
/// @dev if vesting factor is not set, return 0. `_lastMintTimestamp` is set when vesting factor is set
if (_lastMintTimestamp == 0) { return 0; }
mintAmount = TempleMath.mulDivRound(
@> (block.timestamp - _lastMintTimestamp) * (MAX_SUPPLY), vestingFactorCache.numerator,
@> vestingFactorCache.denominator,
false
);
if (totalSupplyCache + mintAmount > MAX_SUPPLY) {
unchecked {
mintAmount = MAX_SUPPLY - totalSupplyCache;
}
}
}

The vestingFactor is not immutable and can be updated by calling the setVestingFactor() function (only callable by the contract owner):

/**
* @notice Set vesting factor
* @param _factor Vesting factor
*/
function setVestingFactor(VestingFactor calldata _factor) external override onlyOwner {
if (_factor.numerator == 0 || _factor.denominator == 0) { revert CommonEventsAndErrors.ExpectedNonZero(); }
if (_factor.numerator > _factor.denominator) { revert CommonEventsAndErrors.InvalidParam(); }
vestingFactor = _factor;
/// @dev initialize
if (lastMintTimestamp == 0) { lastMintTimestamp = uint32(block.timestamp); }
emit VestingFactorSet(_factor.numerator, _factor.denominator);
}

However, when the vestingFactor is updated, it affects the tokens that have been "released" according to the original vestingFactor, but that haven't been minted yet, i.e., released tokens since the last call to mint(). Another way to view the problem is to think about the following simple invariant:

Given that the vesting schedule is linear, tokens that have been already released cannot be taken back.

In that context, updating the vestingFactor should only affect the vesting rate of future tokens, and not the tokens that have already been released. This is not the case, as it is demonstrated in the Proof of concept below.

Proof of concept

The following test (written in Foundry) illustrates how setting a new vestingFactor alters the output from getMintAmount(), which are the tokens available to mint at any given time. Updating the vesting factor should not alter the tokens that have been released, and therefore should not alter the output of getMintAmount(), yet it does.

To run the test, add it to the test contract TempleGoldTest, in the test file: protocol/test/forge/templegold/TempleGold.t.sol, and then run forge test --mt test_setVestingPeriodAltersGetMintAmount -vvv.

function test_setVestingPeriodAltersGetMintAmount() public {
// setting some initial reasonable values for the vesting factor
ITempleGold.VestingFactor memory factor = ITempleGold.VestingFactor(1 seconds, 90 days);
vm.prank(executor);
templeGold.setVestingFactor(factor);
​
// let some time pass so that mintAmount is greater than zero, but DON'T MINT YET
skip(10 days);
assertTrue(templeGold.canDistribute());
​
uint256 mintAmountBefore = templeGold.getMintAmount();
assertGt(mintAmountBefore, 0);
​
// Now, setting a new vesting factor should only affect how future tokens are released, not past tokens
// therefore, setting a new vesting factor should not alter the output of getMintAmount()
ITempleGold.VestingFactor memory newFactor = ITempleGold.VestingFactor(1 seconds, 180 days);
vm.prank(executor);
templeGold.setVestingFactor(newFactor);
​
// however, a new call to getMintAmount will return a new value
uint256 mintAmountAfter = templeGold.getMintAmount();
​
// note: with the current implementation, the following assertion will revert
assertEq(mintAmountBefore, mintAmountAfter, "the mintAmount should not change when updating the vestingFactor");
}

Impact

When the vestingFactor is updated, it changes retrospectively the tokens that have been released according to the original vesting rate, but have not been minted yet (tokens released since the last call to mint()). In both cases, it will generate a step-wise change in the released schedule, instead of only a change in slope.

  • If the new vestingFactor is greater than the original, this step would suddenly release extra tokens.

  • If the new vestingFactor is lower than the original, it will effectively "unrelease already released templeGold tokens", reducing the rewards for next period in the TempleGoldStaking and DaiGoldAuction contracts.

Note: the impact is smaller the smaller the duration between block.timestamp and lastMintTime.

Tools Used

Manual review, foundry for the PoC

Recommendations

Several ways of mitigating the issue:

  1. Not changing the vesting rate. Remove the function or never call it

  2. Only changing the vesting rate right after calling mint(), to minimize the impact of the miscalculation. To enforce this, add the following requirement:

/**
* @notice Set vesting factor
* @param _factor Vesting factor
*/
function setVestingFactor(VestingFactor calldata _factor) external override onlyOwner {
if (_factor.numerator == 0 || _factor.denominator == 0) { revert CommonEventsAndErrors.ExpectedNonZero(); }
if (_factor.numerator > _factor.denominator) { revert CommonEventsAndErrors.InvalidParam(); }
+ // feel free to use constants instead of hardcoded "1 hours", and use custom errors instead of strings.
+ if ((lastMintTimestamp > 0) && (block.timestamp - lastMintTimestamp > 1 hours)) {
+ revert("A new vesting factor can only be updated right after mint");
+ }
vestingFactor = _factor;
/// @dev initialize
if (lastMintTimestamp == 0) { lastMintTimestamp = uint32(block.timestamp); }
emit VestingFactorSet(_factor.numerator, _factor.denominator);
}
Updates

Lead Judging Commences

inallhonesty Lead Judge 12 months ago
Submission Judgement Published
Validated
Assigned finding tags:

When the operators call `TempleGold::setVestingFactor` to modify the `VestingFactor`, the tokens accumulated based on the previous factor are not resolved.

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.