VTVL contest
Findings & Analysis Report
2022-11-01
Table of contents
- Summary
- Scope
- Severity Criteria
-
- [M-01] Supply cap of
VariableSupplyERC20Tokenis not properly enforced - [M-02]
_baseVestedAmount()andvestedAmount()Return Incorrect Historical Values - [M-03] Possible DoS on
vestingRecipientsdue to lack of disposal mechanism - [M-04] not able to create claim
- [M-05] Tokens with lower number of decimals can result in postponed linear vesting for user
- [M-06] Variable balance token causing fund lock and loss
- [M-07] Vesting Schedule Start and End Time can be Set in the Past
- [M-08] Two address tokens can be withdrawn by the admin even if they are vested
- [M-09]
_releaseIntervalSecsis not validated - [M-10] Reentrancy may allow an admin to steal funds
- [M-01] Supply cap of
-
- Summary
- G‑01 Save gas by not requring non-zero interval if no linear amount
- G‑02 Results of calls to
_msgSender()not cached - G‑03 Using
calldatainstead ofmemoryfor read-only arguments inexternalfunctions saves gas - G‑04 State variables should be cached in stack variables rather than re-reading them from storage
- G‑05
<x> += <y>costs more gas than<x> = <x> + <y>for state variables - G‑06 Add
unchecked {}for subtractions where the operands cannot underflow because of a previousrequire()orif-statement - G‑07
++i/i++should beunchecked{++i}/unchecked{i++}when it is not possible for them to overflow, as is the case when used infor- andwhile-loops - G‑08 Optimize names to save gas
- G‑09 Using
bools for storage incurs overhead - G‑10
++icosts less gas thani++, especially when it’s used infor-loops (--i/i--too) - G‑11 Splitting
require()statements that use&&saves gas - G‑12 Don’t compare boolean expressions to boolean literals
- G‑13 Use custom errors rather than
revert()/require()strings to save gas - G‑14 Functions guaranteed to revert when called by normal users can be marked
payable - G‑15 Don’t use
_msgSender()if not supporting EIP-2771
- Disclosures
Overview
About C4
Code4rena (C4) is an open organization consisting of security researchers, auditors, developers, and individuals with domain expertise in smart contracts.
A C4 audit contest is an event in which community participants, referred to as Wardens, review, audit, or analyze smart contract logic in exchange for a bounty provided by sponsoring projects.
During the audit contest outlined in this document, C4 conducted an analysis of the VTVL smart contract system written in Solidity. The audit contest took place between September 20—September 23 2022.
Wardens
208 Wardens contributed reports to the VTVL contest:
- pashov
- sorrynotsorry
- Respx
- fatherOfBlocks
- m9800
- wagmi
- CertoraInc (egjlmn1, OriDabush, ItayG, shakedwinder, and RoiEvenHaim)
- csanuragjain
- TomJ
- hansfriese
- ayeslick
- Lambda
- rbserver
- KIntern_NA (TrungOre and duc)
- neko_nyaa
- rokinot
- 0xSky
- neumo
- bin2chen
- Trust
- wastewa
- datapunk
- 0xhunter
- dipp
- wuwe1
- IllIllI
- Ruhum
- obront
- RustyRabbit
- 0xSmartContract
- 0xA5DF
- pedroais
- pcarranzav
- ElKu
- Czar102
- sashik_eth
- pauliax
- 0x52
- 0xdapper
- eierina
- AkshaySrivastav
- JohnSmith
- __141345__
- djxploit
- 0xDecorativePineapple
- zzzitron
- hyh
- MiloTruck
- rotcivegaf
- JLevick
- Aymen0909
- supernova
- 0x4non
- Chom
- ak1
- 0xNazgul
- 0x1f8b
- rajatbeladiya
- joestakey
- berndartmueller
- c3phas
- lukris02
- pfapostol
- ajtra
- imare
- cryptostellar5
- Deivitto
- gogo
- Bnke0x0
- oyc_109
- JC
- durianSausage
- Diana
- brgltd
- ladboy233
- Tomo
- Rolezn
- seyni
- 0xbepresent
- peanuts
- OptimismSec (sseefried and tofunmi)
- d3e4
- RockingMiles (robee and pants)
- Waze
- tnevler
- Funen
- a12jmx
- prasantgupta52
- Sm4rty
- martin
- delfin454000
- leosathya
- RaymondFam
- Rohan16
- erictee
- millersplanet
- aysha
- ChristianKuri
- V_B (Barichek and vlad_bochok)
- CodingNameKiki
- karanctf
- ret2basic
- medikko
- slowmoses
- ReyAdmirado
- B2
- peiw
- 0x040
- carrotsmuggler
- ikbkln
- async
- sach1r0
- rvierdiiev
- eighty
- ignacio
- bobirichman
- got_targ
- nalus
- cryptphi
- SooYa
- tibthecat
- natzuu
- indijanc
- 2997ms
- exd0tpy
- MasterCookie
- StevenL
- bulej93
- Diraco
- 0v3rf10w
- 0x85102
- Yiko
- Bahurum
- chatch
- 0xmatt
- cccz
- innertia
- reassor
- zzykxx
- 0x5rings
- ubermensch
- 0xf15ers (remora and twojoy)
- Dravee
- JohnnyTime
- Aeros
- yongskiws
- romand
- dic0de
- peritoflores
- sikorico
- Margaret
- pedr02b2
- ch13fd357r0y3r
- Junnon
- Atarpara
- jag
- DimitarDimitrov
- adriro
- zishansami
- ch0bu
- SnowMan
- Saintcode_
- 0xsam
- gianganhnguyen
- WilliamAmbrozic
- Tomio
- samruna
- yaemsobak
- emrekocak
- Tadashi
- tgolding55
- Ocean_Sky
- caventa
- beardofginger
- dharma09
- malinariy
- lucacez
- subtle77
- 0xDanielC
- mics
- w0Lfrum
- hxzy
- Amithuddar
- Tagir2003
- 0xc0ffEE
- Satyam_Sharma
- Noah3o6
- jpserrat
- Matin
- Sta1400
- mrpathfindr
- francoHacker
- cRat1st0s
- cryptonue
- Franfran
- GimelSec (rayn and sces60107)
- JGcarv
- Soosh
This contest was judged by 0xean.
Final report assembled by liveactionllama.
Summary
The C4 analysis yielded an aggregated total of 12 unique vulnerabilities. Of these vulnerabilities, 2 received a risk rating in the category of HIGH severity and 10 received a risk rating in the category of MEDIUM severity.
Additionally, C4 analysis included 135 reports detailing issues with a risk rating of LOW severity or non-critical. There were also 141 reports recommending gas optimizations.
All of the issues presented here are linked back to their original finding.
Scope
The code under review can be found within the C4 VTVL contest repository, and is composed of 4 smart contracts written in the Solidity programming language and includes 239 lines of Solidity code.
Severity Criteria
C4 assesses the severity of disclosed vulnerabilities according to a methodology based on OWASP standards.
Vulnerabilities are divided into three primary risk categories: high, medium, and low/non-critical.
High-level considerations for vulnerabilities span the following key areas when conducting assessments:
- Malicious Input Handling
- Escalation of privileges
- Arithmetic
- Gas use
Further information regarding the severity criteria referenced throughout the submission review process, please refer to the documentation provided on the C4 website.
High Risk Findings (2)
[H-01] Loss of vested amounts
Submitted by eierina, also found by 0x52, 0xA5DF, 0xdapper, ElKu, obront, pauliax, pcarranzav, pedroais, rbserver, Ruhum, RustyRabbit, and TomJ
VTVLVesting.sol#L418
VTVLVesting.sol#L147-L151
VTVLVesting.sol#L364
Vesting is a legal term that means the point in time where property is earned or gained by some person.
The VTVLVesting contract defines:
- a start time (
Claim::startTimestamp) and an end time (Claim::endTimestamp) at which vesting starts and ends for a entitled user - the calculated points in time when the fractions of the total amount are released and therefore can be withdrawn (which are defined by
Claim::releaseIntervalSecs).
The entitled user can either withdraw after each interval elapses, or after the whole vesting period is over or any variant of the two options.
The administrator of the contract can revoke the claim for a user at any time, which for vesting assets is expected. For example an employee with a vesting stock allocation of 1000 shares vesting at each quarter over a period of 4 years, may resign after 2 years and therefore the only half of the shares would be vested and therefore sold by the employee. The employee can either sell them at each quarter, or before, or after resigning, in any case the half of the shares have vested and are by legal right owned by the employee.
The VTVLContract revoke has the following defects:
- it ignores the amount already vested and now yet withdrawn
- if called, say half-way the total period, just after claimer withdraws the already vested amount, it revokes only the right to vest the remaining part in future.
- if called, say half-way the total period, right before the claimer withdraws the already vested amount, it revokes both the already vested amount and the right to vest the remaining part in future.
Raising as high impact because it actually causes:
- loss of already vested amounts of a user with a valid claim that has already righteously vested a part but not withdrawn
- different outcomes depending on the order in which withdraw and revokeClaim functions are called which means that one of the two behavoiurs is certainly in conflict with the other causing a loss on one of the two sides, contract or claimer (by definition of Vesting rights, the claimer).
- lack of trust by the potential claimers/users whch can be at any time deprived of righteously vested amounts.
Proof of Concept
The following two tests prove the behaviour difference when the order by which revokeClaim vs withdraw are called, whch shows that the vesting right is not guaranteed.
// NOTE: USES ORIGINAL REVOKE BEHAVIOUR
it('sample revoke use case USER LOSE: employee withdraw immediately after resignation', async () => {
const {tokenContract, vestingContract} = await createPrefundedVestingContract({tokenName, tokenSymbol, initialSupplyTokens});
const startTimestamp = await getLastBlockTs() + 100;
const endTimestamp = startTimestamp + 2000;
const terminationTimestamp = startTimestamp + 1000 + 50; // half-way vesting, plus half release interval which shall be discarded
const releaseIntervalSecs = 100;
await vestingContract.createClaim(owner2.address, startTimestamp, endTimestamp, cliffReleaseTimestamp, releaseIntervalSecs, linearVestAmount, cliffAmount);
// move clock to termination timestamp (half-way the vesting period plus a bit, but less than release interval seconds)
await ethers.provider.send("evm_mine", [terminationTimestamp]);
let availableAmt = await vestingContract.claimableAmount(owner2.address)
// revoke the claim preserving the "already vested but not yet withdrawn amount"
await (await vestingContract.revokeClaim(owner2.address)).wait();
let userBalanceBefore = await tokenContract.balanceOf(owner2.address);
await expect(vestingContract.connect(owner2).withdraw()).to.be.revertedWith('NO_ACTIVE_CLAIM');
let userBalanceAfter = await tokenContract.balanceOf(owner2.address);
// move the clock to the programmed end of vesting period
await ethers.provider.send("evm_mine", [endTimestamp]);
// cliffTimestamp < startTimestamp < terminationTimestamp, hence expected cliffAmount + (1/2 * anlinearVestAmount)
let expectedVestedAmount = cliffAmount.add(linearVestAmount.div(2));
// RESIGNING EMPLOYEE LOSES HIS VESTED AMOUNT BECAUSE OF WITHDRAWING IMMEDIATELY AFTER RESIGNATION
expect(userBalanceAfter.sub(userBalanceBefore)).to.be.equal(0);
// VTVLVesting CONTRACT TOOK ALREADY VESTED AMOUNT FROM OWNER2
expect(await vestingContract.finalClaimableAmount(owner2.address)).to.be.equal(0);
});
// NOTE: USES ORIGINAL REVOKE BEHAVIOUR
it('sample revoke use case USER WIN: employee withdraw immediately before resignation', async () => {
const {tokenContract, vestingContract} = await createPrefundedVestingContract({tokenName, tokenSymbol, initialSupplyTokens});
const startTimestamp = await getLastBlockTs() + 100;
const endTimestamp = startTimestamp + 2000;
const terminationTimestamp = startTimestamp + 1000 + 50; // half-way vesting, plus half release interval which shall be discarded
const releaseIntervalSecs = 100;
await vestingContract.createClaim(owner2.address, startTimestamp, endTimestamp, cliffReleaseTimestamp, releaseIntervalSecs, linearVestAmount, cliffAmount);
// move clock to termination timestamp (half-way the vesting period plus a bit, but less than release interval seconds)
await ethers.provider.send("evm_mine", [terminationTimestamp]);
let userBalanceBefore = await tokenContract.balanceOf(owner2.address);
await (await vestingContract.connect(owner2).withdraw()).wait();
let userBalanceAfter = await tokenContract.balanceOf(owner2.address);
// revoke the claim preserving the "already vested but not yet withdrawn amount"
await (await vestingContract.revokeClaim(owner2.address)).wait();
// move the clock to the programmed end of vesting period
await ethers.provider.send("evm_mine", [endTimestamp]);
console.log(userBalanceAfter.sub(userBalanceBefore));
// RESIGNING EMPLOYEE RECEIVES HIS VESTED AMOUNT BY WITHDRAWING IMMEDIATELY BEFORE RESIGNATION
expect(userBalanceAfter.sub(userBalanceBefore)).to.be.greaterThan(0);
expect(await vestingContract.finalClaimableAmount(owner2.address)).to.be.equal(0);
});Recommended Mitigation Steps
Below are, in order, a test and a diff/patch for a proposed fix. The proposed fix is just an idea at how to fix, or in other words, a way to preserve the already vested amount when claim is revoked.
The diff/patch add a deactivationTimestamp to claim, and a new revokeClaimProper that shall replace the revokeClaim function to correct the behaviour. The deactivationTimestamp is used to track the deactivation time for the claim in order to preserve the amount vested so far and allow the user to withdraw the amount righteously earned so far. The _baseVestedAmount and hasActiveClaim have been updated to do proper math when isActive is false but deactivationTimestamp is greater than 0.
The finalVestedAmount has been update to show the “what would be” amount if the vesting would have reached the claim endTimestamp while the finalClaimableAmount takes into consideration the deactivationTimestamp if the claim has been revoked.
The test shows that the already vested amount (cliff + half way linear vesting) is preserved.
diff --git a/contracts/VTVLVesting.sol b/contracts/VTVLVesting.sol
index 133f19f..7ab955c 100644
--- a/contracts/VTVLVesting.sol
+++ b/contracts/VTVLVesting.sol
@@ -34,6 +34,7 @@ contract VTVLVesting is Context, AccessProtected {
// Gives us a range from 1 Jan 1970 (Unix epoch) up to approximately 35 thousand years from then (2^40 / (365 * 24 * 60 * 60) ~= 35k)
uint40 startTimestamp; // When does the vesting start (40 bits is enough for TS)
uint40 endTimestamp; // When does the vesting end - the vesting goes linearly between the start and end timestamps
+ uint40 deactivationTimestamp;
uint40 cliffReleaseTimestamp; // At which timestamp is the cliffAmount released. This must be <= startTimestamp
uint40 releaseIntervalSecs; // Every how many seconds does the vested amount increase.
@@ -108,7 +109,7 @@ contract VTVLVesting is Context, AccessProtected {
// We however still need the active check, since (due to the name of the function)
// we want to only allow active claims
- require(_claim.isActive == true, "NO_ACTIVE_CLAIM");
+ require(_claim.isActive == true || _claim.deactivationTimestamp > 0, "NO_ACTIVE_CLAIM");
// Save gas, omit further checks
// require(_claim.linearVestAmount + _claim.cliffAmount > 0, "INVALID_VESTED_AMOUNT");
@@ -144,20 +145,20 @@ contract VTVLVesting is Context, AccessProtected {
@param _claim The claim in question
@param _referenceTs Timestamp for which we're calculating
*/
- function _baseVestedAmount(Claim memory _claim, uint40 _referenceTs) internal pure returns (uint112) {
+ function _baseVestedAmount(Claim memory _claim, uint40 _referenceTs, uint40 vestEndTimestamp) internal pure returns (uint112) {
uint112 vestAmt = 0;
-
- // the condition to have anything vested is to be active
- if(_claim.isActive) {
+
+ if(_claim.isActive || _claim.deactivationTimestamp > 0) {
// no point of looking past the endTimestamp as nothing should vest afterwards
// So if we're past the end, just get the ref frame back to the end
- if(_referenceTs > _claim.endTimestamp) {
- _referenceTs = _claim.endTimestamp;
+ if(_referenceTs > vestEndTimestamp) {
+ _referenceTs = vestEndTimestamp;
}
// If we're past the cliffReleaseTimestamp, we release the cliffAmount
// We don't check here that cliffReleaseTimestamp is after the startTimestamp
- if(_referenceTs >= _claim.cliffReleaseTimestamp) { // @audit is _claim.require(cliffReleaseTimestamp < _claim.endTimestamp) ?
+ if(_referenceTs >= _claim.cliffReleaseTimestamp) { // @audit note cliffReleaseTimestamp cannot? be zero without cliffamoutn being zero
+ // @audit NOTE: (cliffReleaseTimestamp is always <= _startTimestamp <= endTimestamp, or 0 if no vesting)
vestAmt += _claim.cliffAmount;
}
@@ -195,7 +196,8 @@ contract VTVLVesting is Context, AccessProtected {
*/
function vestedAmount(address _recipient, uint40 _referenceTs) public view returns (uint112) {
Claim storage _claim = claims[_recipient];
- return _baseVestedAmount(_claim, _referenceTs);
+ uint40 vestEndTimestamp = _claim.isActive ? _claim.endTimestamp : _claim.deactivationTimestamp;
+ return _baseVestedAmount(_claim, _referenceTs, vestEndTimestamp);
}
/**
@@ -205,7 +207,18 @@ contract VTVLVesting is Context, AccessProtected {
*/
function finalVestedAmount(address _recipient) public view returns (uint112) {
Claim storage _claim = claims[_recipient];
- return _baseVestedAmount(_claim, _claim.endTimestamp);
+ return _baseVestedAmount(_claim, _claim.endTimestamp, _claim.endTimestamp);
+ }
+
+ /**
+ @notice Calculates how much wil be possible to claim at the end of vesting date, by subtracting the already withdrawn
+ amount from the vestedAmount at this moment. Vesting date is either the end timestamp or the deactivation timestamp.
+ @param _recipient - The address for whom we're calculating
+ */
+ function finalClaimableAmount(address _recipient) external view returns (uint112) {
+ Claim storage _claim = claims[_recipient];
+ uint40 vestEndTimestamp = _claim.isActive ? _claim.endTimestamp : _claim.deactivationTimestamp;
+ return _baseVestedAmount(_claim, vestEndTimestamp, vestEndTimestamp) - _claim.amountWithdrawn;
}
/**
@@ -214,7 +227,8 @@ contract VTVLVesting is Context, AccessProtected {
*/
function claimableAmount(address _recipient) external view returns (uint112) {
Claim storage _claim = claims[_recipient];
- return _baseVestedAmount(_claim, uint40(block.timestamp)) - _claim.amountWithdrawn;
+ uint40 vestEndTimestamp = _claim.isActive ? _claim.endTimestamp : _claim.deactivationTimestamp;
+ return _baseVestedAmount(_claim, uint40(block.timestamp), vestEndTimestamp) - _claim.amountWithdrawn;
}
/**
@@ -280,6 +294,7 @@ contract VTVLVesting is Context, AccessProtected {
Claim memory _claim = Claim({
startTimestamp: _startTimestamp,
endTimestamp: _endTimestamp,
+ deactivationTimestamp: 0,
cliffReleaseTimestamp: _cliffReleaseTimestamp,
releaseIntervalSecs: _releaseIntervalSecs,
cliffAmount: _cliffAmount,
@@ -436,6 +451,30 @@ contract VTVLVesting is Context, AccessProtected {
emit ClaimRevoked(_recipient, amountRemaining, uint40(block.timestamp), _claim);
}
+ function revokeClaimProper(address _recipient) external onlyAdmin hasActiveClaim(_recipient) {
+ // Fetch the claim
+ Claim storage _claim = claims[_recipient];
+ // Calculate what the claim should finally vest to
+ uint112 finalVestAmt = finalVestedAmount(_recipient);
+
+ // No point in revoking something that has been fully consumed
+ // so require that there be unconsumed amount
+ require( _claim.amountWithdrawn < finalVestAmt, "NO_UNVESTED_AMOUNT");
+
+ _claim.isActive = false;
+ _claim.deactivationTimestamp = uint40(block.timestamp);
+
+ uint112 vestedSoFarAmt = vestedAmount(_recipient, uint40(block.timestamp));
+ // The amount that is "reclaimed" is equal to the total allocation less what was already
+ // vested without the part that was already withdrawn.
+ uint112 amountRemaining = finalVestAmt - (vestedSoFarAmt - _claim.amountWithdrawn);
+
+ numTokensReservedForVesting -= amountRemaining; // Reduces the allocation
+
+ // Tell everyone a claim has been revoked.
+ emit ClaimRevoked(_recipient, amountRemaining, uint40(block.timestamp), _claim);
+ }
+
/**
@notice Withdraw a token which isn't controlled by the vesting contract.
@dev This contract controls/vests token at "tokenAddress". However, someone might send a different token. lawrencehui (VTVL) confirmed and commented:
Thank you warden for the findings. We did think about adding a grace withdrawing period to further strengthen the users’ trust to the admin. I would argue that severity is medium in the case as in practical sense we would assume admin will inform the receivers upon revocation and therefore withdrawAdmin was designed to be separated from revokeClaim.
I acknowledge that some malicious admin might abuse this right and to claimed the receiver’s already earned token before they claimed (as described in the scenario in this findings) and therefore we will consider adding the grace period to restrict admin to act maliciously.
I am going to stick with High, even a non malicious admin would have no choice but to kindly ask a user to claim before they revoked all their other tokens. If the user didn’t comply, the admin has no option but to either “steal” their tokens or allow them to keep vesting.
[H-02] Permanent freeze of vested tokens due to overflow in _baseVestedAmount
Submitted by Trust, also found by 0xSky, bin2chen, CertoraInc, hansfriese, KIntern_NA, neko_nyaa, neumo, rokinot, and wastewa
The _baseVestedAmount() function calculates vested amount for some (claim, timestamp) pair. It is wrapped by several functions, like vestedAmount, which is used in withdraw() to calculate how much a user can retrieve from their claim. Therefore, it is critical that this function will calculate correctly for users to receive their funds.
Below is the calculation of the linear vest amount:
uint112 linearVestAmount = _claim.linearVestAmount * truncatedCurrentVestingDurationSecs / finalVestingDurationSecs;Importantly, _claim.linearVestAmount is of type uint112 and truncatedCurrentVestingDurationSecs is of type uint40. Using compiler >= 0.8.0, the product cannot exceed uint112 or else the function reverts due to overflow. In fact, we can show that uint112 is an inadequate size for this calculation.
The max value for uint112 is 5192296858534827628530496329220096.
Seconds in year = 3600 * 24 * 365 = 31536000
Tokens that inherit from ERC20 like the ones used in VTVL have 18 decimal places -> 1000000000000000000
This means the maximum number of tokens that are safe to vest for one year is 2**112 / 10e18 / (3600 * 24 * 365) = just 16,464,665 tokens.
This is definitely not a very large amount and it is expected that some projects will mint a similar or larger amount for vesting for founders / early employees. For 4 year vesting, the safe amount drops to 4,116,166.
Projects that are not forewarned about this size limit are likely to suffer from freeze of funds of employees, which will require very patchy manual revocation and restructuring of the vesting to not overflow.
Impact
Employees/founders do not have access to their vested tokens.
Proof of Concept
Below is a test that demonstrates the overflow issue, 1 year after 17,000,000 tokens have matured.
describe('Long vest fail', async () => {
let vestingContract: VestingContractType;
// Default params
// linearly Vest 10000, every 1s, between TS 1000 and 2000
// additionally, cliff vests another 5000, at TS = 900
const recipientAddress = await randomAddress();
const startTimestamp = BigNumber.from(1000);
const endTimestamp = BigNumber.from(1000 + 3600 * 24 * 365);
const midTimestamp = BigNumber.from(1000 + (3600 * 24 * 365) / 2);
const cliffReleaseTimestamp = BigNumber.from(0);
const linearVestAmount = BigNumber.from('170000000000000000000000000');
const cliffAmount = BigNumber.from(0);
const releaseIntervalSecs = BigNumber.from(5);
before(async () => {
const {vestingContract: _vc} = await createPrefundedVestingContract({tokenName, tokenSymbol, initialSupplyTokens});
vestingContract = _vc;
await vestingContract.createClaim(recipientAddress, startTimestamp, endTimestamp, cliffReleaseTimestamp, releaseIntervalSecs, linearVestAmount, cliffAmount);
});
it('half term works', async() => {
expect(await vestingContract.vestedAmount(recipientAddress, midTimestamp)).to.be.equal('85000000000000000000000000');
});
it('full term fails', async() => {
// Note: at exactly the cliff time, linear vested amount won't yet come in play as we're only at second 0
await expect(vestingContract.vestedAmount(recipientAddress, endTimestamp)).to.be.revertedWithPanic(0x11
);
});
});Tools Used
Manual audit, hardhat / chai.
Recommended Mitigation Steps
Perform the intermediate calculation of linearVestAmount using the uint256 type.
uint112 linearVestAmount = uint112( uint256(_claim.linearVestAmount) * truncatedCurrentVestingDurationSecs / finalVestingDurationSecs);lawrencehui (VTVL) confirmed and commented:
This finding is very useful and appreciate all wardens that flagged the potential risk of overflowing.
Medium Risk Findings (10)
[M-01] Supply cap of VariableSupplyERC20Token is not properly enforced
Submitted by Czar102, also found by __141345__, 0xbepresent, 0xDecorativePineapple, 0xmatt, 0xNazgul, 0xSky, adriro, ajtra, Atarpara, Bahurum, bin2chen, cccz, cRat1st0s, cryptonue, d3e4, DimitarDimitrov, Franfran, GimelSec, innertia, jag, JGcarv, JLevick, joestakey, Junnon, neumo, obront, OptimismSec, pashov, pauliax, pcarranzav, peanuts, rajatbeladiya, rbserver, reassor, Rolezn, Ruhum, seyni, Soosh, Tomo, Trust, wagmi, zzykxx, and zzzitron
VariableSupplyERC20Token.sol#L36-L46
The admin of the token is not constrained to minting maxSupply_, they can mint any number of tokens.
Proof of Concept
// If we're using maxSupply, we need to make sure we respect it
// mintableSupply = 0 means mint at will
if(mintableSupply > 0) {
require(amount <= mintableSupply, "INVALID_AMOUNT");
// We need to reduce the amount only if we're using the limit, if not just leave it be
mintableSupply -= amount;
}The logic is as follows: if the amount that can be minted is zero, treat this as an infinite mint. Else require that the minted amount is not larger than mintable supply.
One can note that it is possible to mint all mintable supply. Then the mintable supply will be 0 which will be interpreted as infinity and any number of tokens will be possible to be minted.
Recommended Mitigation Steps
Treat 2 ** 256 - 1 as infinity instead of 0.
0xean (judge) decreased severity to Medium and commented:
The warden’s logic is correct, but given that this is behind an admin only flag, there are some external factors that would need to come into play for this to be realized. Downgrading to Medium severity.
[M-02] _baseVestedAmount() and vestedAmount() Return Incorrect Historical Values
Submitted by Respx, also found by m9800
VTVLVesting.sol#L183-L187
VTVLVesting.sol#L198
As the comments in _baseVestedAmount() explain, once there is any _claim.amountWithdrawn, it will be returned if it is greater than the calculated value vestAmt. However, vestAmt takes account of time, _referenceTs, whereas _claim.amountWithdrawn is always the amount withdrawn to date. Therefore, for all historical values below _claim.amountWithdrawn, including timestamps before _claim.startTimestamp and before _claim.cliffReleaseTimestamp, _claim.amountWithdrawn will be returned.
Impact
Given that VTVL is intended to be an accessible platform for use by a wide variety of users, this behaviour does create a security risk. Consider these scenarios:
- A protocol relies on VTVL as an off-the-shelf solution for vesting, but builds other systems (escrow, NFT grants, access, airdrops) that work by checking the value of
vestedAmount(). Airdrops are especially likely to be interested in historical values. These values would be distorted by how much users have claimed and so would result in an undesirable distribution of resources. - Even if the above does not occur, consider that VTVL might be passed over as a vesting solution precisely because its historical data is inaccurate.
- A contract could be built that inherits from
VTVLVestingand attempts to use_baseVestedAmount()(which isinternaland so can be used by inheriting contracts). The inheriting contract might apportion rewards based on historical usage. - VTVL itself might wish to inherit from
VTVLVestingin future.
Proof of Concept
diff --git a/test/VTVLVesting.ts b/test/VTVLVestingPOC.ts
index bb609fb..073e53f 100644
--- a/test/VTVLVesting.ts
+++ b/test/VTVLVestingPOC.ts
@@ -500,14 +500,37 @@ describe('Revoke Claim', async () => {
const recipientAddress = await randomAddress();
const [owner, owner2] = await ethers.getSigners();
- it('allows admin to revoke a valid claim', async () => {
+ it('POC: WITHDRAWN DATA IS UNRELIABLE', async () => {
const {vestingContract} = await createPrefundedVestingContract({tokenName, tokenSymbol, initialSupplyTokens});
- await vestingContract.createClaim(recipientAddress, startTimestamp, endTimestamp, cliffReleaseTimestamp, releaseIntervalSecs, linearVestAmount, cliffAmount);
+ const startTimestamp2 = startTimestamp.add(releaseIntervalSecs.mul(100));
+ const endTimestamp2 = endTimestamp.add(releaseIntervalSecs.mul(100));
+ const cliffReleaseTimestamp2 = cliffReleaseTimestamp.add(releaseIntervalSecs.mul(100));
+ await vestingContract.createClaim(owner2.address, startTimestamp2, endTimestamp2, cliffReleaseTimestamp2, releaseIntervalSecs, linearVestAmount, cliffAmount);
+
+ // Fast forward to middle of claim
+ const halfWay = startTimestamp2.toNumber() + (endTimestamp2.toNumber()-startTimestamp2.toNumber())/2;
+ await ethers.provider.send("evm_mine", [halfWay]);
+
+ let vestAmt = await vestingContract.vestedAmount(owner2.address, startTimestamp);
+ console.log("NO WITHDRAWAL, BEFORE VEST START: ",vestAmt.toString());
+ vestAmt = await vestingContract.vestedAmount(owner2.address, startTimestamp2);
+ console.log("NO WITHDRAWAL, AT VEST START: ",vestAmt.toString());
+ vestAmt = await vestingContract.vestedAmount(owner2.address, halfWay);
+ console.log("NO WITHDRAWAL, HALF WAY THROUGH VEST: ",vestAmt.toString());
+ vestAmt = await vestingContract.vestedAmount(owner2.address, endTimestamp2);
+ console.log("NO WITHDRAWAL, AT VEST END: ",vestAmt.toString());
+
+ await (await vestingContract.connect(owner2).withdraw()).wait();
- (await vestingContract.revokeClaim(recipientAddress)).wait();
+ vestAmt = await vestingContract.vestedAmount(owner2.address, startTimestamp);
+ console.log("WITHDRAWAL, BEFORE VEST START: ",vestAmt.toString());
+ vestAmt = await vestingContract.vestedAmount(owner2.address, startTimestamp2);
+ console.log("WITHDRAWAL, AT VEST START: ",vestAmt.toString());
+ vestAmt = await vestingContract.vestedAmount(owner2.address, halfWay);
+ console.log("WITHDRAWAL, HALF WAY THROUGH VEST: ",vestAmt.toString());
+ vestAmt = await vestingContract.vestedAmount(owner2.address, endTimestamp2);
+ console.log("WITHDRAWAL, AT VEST END: ",vestAmt.toString());
- // Make sure it gets reverted
- expect(await (await vestingContract.getClaim(recipientAddress)).isActive).to.be.equal(false);
});
it('prohibits a random user from revoking a valid claim', async () => {Recommended Mitigation Steps
For active claims, there is no reason to consider _claim.amountWithdrawn, as it will always have been below or equal to vestAmt at any point in time. So only consider vestAmt for inactive claims. For them, return the lowest of vestAmt and _claim.amountWithdrawn. This will keep the values monotonic with time without distorting the historical values. It will act as though _claim.amountWithdrawn was withdrawn and the claim was revoked in the block when vestAmt reached _claim.amountWithdrawn. That is a distortion,0xean but it is required to provide monotonicity.
Good find.
[M-03] Possible DoS on vestingRecipients due to lack of disposal mechanism
Submitted by fatherOfBlocks, also found by wagmi
VTVLVesting.sol#L224
VTVLVesting.sol#L245
VTVLVesting.sol#L302
VTVLVesting.sol#L317
When the smart contracts start to be used, the variable in storage vestingRecipients will start to be filled with addresses, as there is no mechanism to eliminate elements, this will cause the allVestingRecipients() function to generate a DoS yes has many addressess.
Recommended Mitigation Steps
In the withdraw() function you could remove the element from vestingRecipients that no longer has vesting. This would make the variable not grow without reducing elements.
On the fence on this one. I agree with the warden, but in the current implementation
allVestingRecipientsis unused and assumed to be for external, off chain uses so the impact is hard to determine. Going to leave as Medium, pending sponsor review.
lawrencehui (VTVL) confirmed and commented:
I would agree with the warden on the lack of control for an ever-growing array size could be an issue. I will tag this as an enhancement.
On the side, I want to check what is the allowed max size of the array in this case?
2**256 -1? but theoretically calling a large array would exceed the the block gas limit when retrieving it?
@lawrencehui - Yes retrieval will eventually fail, long before you populate the array fully. You could pass in an index range to retrieve portions of the array to avoid this failure mode.
And yes
2**256 -1is my understanding of the theoretical limit.
[M-04] not able to create claim
Submitted by rajatbeladiya, also found by 0x4non, ak1, berndartmueller, CertoraInc, Chom, imare, JLevick, joestakey, JohnSmith, KIntern_NA, obront, rbserver, rotcivegaf, Ruhum, RustyRabbit, and supernova
If admin revoked any recipient’s claim, admin can not create claim for the same recipient because startTimestamp is not updated to initial value on revoke claim.
There will be a need to create a claim again for any reason like: 1) mistakenly revoked claim, 2) wrong info provided to claim, 3) new vesting period starts, etc.
Proof of Concept
- Alice creates claim for Bob
-
Alice revokes claim of Bob
- On
revokeClaim(), claim’sisActivewill be false, butstartTimestampwill remain as it is - VTVLVesting.sol#L418-L437
- On
- Alice tries to create claim for Bob but claim will not create because it has modifier
hasNoClaim()which is checked for claim should not active and it checks forrequire(_claim.startTimestamp == 0, "CLAIM_ALREADY_EXISTS"); - VTVLVesting.sol#L245-L253
- VTVLVesting.sol#L123-L140
Recommended Mitigation Steps
Update startTimestamp to 0 on revokeClaim().
0xean (judge) decreased severity to Low and commented:
Downgrading to low severity. While true, why wouldn’t the employee just use a different address? There is no residual benefit to using the old address (unless it was a smart contract, which the warden doesn’t mention as part of their POC). The sponsor may want to fix this, since the fix is simple, but it poses very little risk and certainly no direct loss of funds.
0xean (judge) increased severity to Medium and commented:
Spent a bit more time thinking about this one and do think that it qualifies as Medium severity since it does affect the availability of the protocol in a number of ways. Going to go ahead and revise to Medium.
lawrencehui (VTVL) acknowledged, but disagreed with severity and commented:
The vesting contract is designed to be created and used in a one-off manner and the revoke function is to prevent any mistakes made upon creation (wrong address / amount / timestamp etc.). In practical sense, if a claim (or the recipient address) is revoked, one (the admin) can always create a new vesting contract with the correct claim parameters.
I therefore think that it is by design that the address is only able be claimable once per vesting contract, in all circumstance, the admin can re-create a new vesting contract to mitigate this issue and therefore this is a low risk / non-critical issue.
I don’t think the tactic of deploying a new contract is the correct one here simply to be able to set up vesting for one botched person or someone whose vesting token amount changes for example. I am going to stick with the Medium severity on this one, but do appreciate the response and thoughts on possible mitigations.
I have explained one of the real use case scenarios where this protocol will fail to serve many. Refer to issue 384.
It is not always contract address or EOA which will decide the identity of a person. Each one will have unique ID. That id is going to be used in all the places.
[M-05] Tokens with lower number of decimals can result in postponed linear vesting for user
Submitted by pashov
In the _baseVestedAmount of VTVLVesting.sol we see the following code
uint40 finalVestingDurationSecs = _claim.endTimestamp - _claim.startTimestamp; // length of the interval
uint112 linearVestAmount = _claim.linearVestAmount * truncatedCurrentVestingDurationSecs / finalVestingDurationSecs;Let’s look at truncatedCurrentVestingDurationSecs as just the duration passed from the start of the vesting period for the PoC (this doesn’t omit important data in this context).
Now think of the following scenario:
We have a token $TKN that has 6 decimals (those are the decimals of both USDT & USDC). We want to distribute 10,000 of those tokens to a user vested over a 10 year period.
10 years in seconds is 315360000 ****(this is finalVestingDurationSecs)
This means that we will distribute 10,000 * 10^6 = 10 000 000 000 fractions of a token for 315360000 seconds, meaning we will distribute 310 fractions of a token each second - this is linearVestAmount
Now, since finalVestingDurationSecs is so big (315360000) it will almost always round linearVestAmount to zero when dividing by it, up until
_claim.linearVestAmount * truncatedCurrentVestingDurationSecs becomes a bigger number than 315360000, but since _claim.linearVestAmount is 310 we will need the current vesting duration to be at least 1 017 290 seconds which is 12 days postponed vesting. 12 days in a moving market can make a big difference if the user was expecting the tokens to start vesting from the first day.
Impact
Unexpected postponing of vesting can result in waiting times for users to receive their must-be-vested tokens. This period can be used by other token holders to dump the token and decrease the price of it, resulting in a loss of capital for the vesting receiver.
Recommended Mitigation Steps
Enforce the contract to work with only 18 decimal tokens with a require statement in the constructor.
0xean (judge) decreased severity to Medium and commented:
Downgrading to Medium, there are a lot of external factors presented here by the warden to line up to a loss of funds.
lawrencehui (VTVL) acknowledged, but disagreed with severity and commented:
I acknowledge the warden’s concern of the rounding, but I think the result of loss of funds is one of the extreme edge cases. I would suggest instead of restricting only to 18 decimal tokens (which is impractical as we would also want to include USDC and USDT for vesting too!), I would implement the rounding checking in the frontend UI and prompt user of potential delay caused by rounding / truncation as described in this issue.
Given that smart contracts can be interacted with in any number of ways (etherscan, programmatically, etc), I don’t think the mitigation negates the risk entirely and am going to stick with the Medium severity here. The wardens demonstrates clearly the way in which this can happen. While it may be a bit outside of the normal vesting schedule expected, I do think it’s valuable to understand the bounds of the math you have employed here.
[M-06] Variable balance token causing fund lock and loss
Submitted by __141345__, also found by 0xDecorativePineapple, CertoraInc, djxploit, hyh, IllIllI, JohnSmith, MiloTruck, rbserver, and zzzitron
VTVLVesting.sol#L295
VTVLVesting.sol#L388
Some ERC20 token’s balance could change, one example is stETH. The balance could become insufficient at the time of withdraw(). User’s fund will be locked due to DoS. The way to take the fund out is to send more token into the contract, causing fund loss to the protocol. And there is no guarantee that until the end time the balance would stay above the needed amount, the lock and loss issue persist.
Proof of Concept
For stETH like tokens, the balanceOf() value might go up or down, even without transfer.
// stETH
function balanceOf(address who) external override view returns (uint256) {
return _shareBalances[who].div(_sharesPerToken);
}In VTVLVesting, the require check for the spot balanceOf() value will pass, but it is possible that as time goes on, the value become smaller and fail the transfer. As a result, the withdraw() call will revert, causing DoS, and lock user’s fund.
// contracts/VTVLVesting.sol
function _createClaimUnchecked() private hasNoClaim(_recipient) {
// ...
require(tokenAddress.balanceOf(address(this)) >= numTokensReservedForVesting + allocatedAmount, "INSUFFICIENT_BALANCE");
// ...
}
function withdraw() hasActiveClaim(_msgSender()) external {
// ...
tokenAddress.safeTransfer(_msgSender(), amountRemaining);
// ...
}Reference
https://etherscan.io/address/0x312ca0592a39a5fa5c87bb4f1da7b77544a91b87#code
Recommended Mitigation Steps
Disallow such kind of variable balance token.
0xean (judge) decreased severity to Medium and commented:
stETH only rebases up, not down. So that is a poor example.
The sponsor’s README does say they will support any ERC20 token, so that could include Fee on Transfer or downward rebasing tokens which could lead to less tokens in the contract than expected and transfers to revert due to balances being lower than expected.
Downgrading to Medium as the external requirement is using these contracts on tokens that are known to have variable supply.
lawrencehui (VTVL) confirmed and commented:
Thanks for reporting this and I will add this as a feature enhancement to cater / avoid for tokens with rebasing supplies.
Question: Is there a straight forward way to detect rebasing tokens? Or on the flip side, restricting erc20 tokens that do not exhibit rebasing behaviour?
@lawrencehui - Great question. There isn’t a great way to detect this functionality in any generic manner unfortunately.
Most contracts that want to handle FOT tokens will do something like (in pseudocode)
uint256 balBefore = ERC20.balanceOf(address(this)); ERC20.transferFrom(...); uint256 balAfter= ERC20.balanceOf(address(this)); uint256 actualBalChange = balAfter - balBefore;Rebasing tokens are different again, and the easiest way to handle them is to create shares to track the internal math. The shares track the % ownership of the entire balance of the contract. Probably more than I can explain here, but would be happy to work with you if this is something you are interested in.
[M-07] Vesting Schedule Start and End Time can be Set in the Past
Submitted by TomJ, also found by ayeslick, csanuragjain, and pashov
There is no check whether _startTimestamp and _endTimestamp is greater than block.timestamp
at VTVLVesting.sol _createClaimUnchecked function. Therefore it is possible for administrators to accidentally create
vesting schedule that starts and ends in the past without noticing it. When administrators does this and this transaction goes
through, then the vesting recipients can withdraw their entire vest amount which is not what administrators intended to do.
Add require check that force _startTimestamp to be greater than block.timestamp.
Team comments as below on line 260
// -> Conclusion: we want to allow this, for founders that might have forgotten to add some users, or to avoid issues with
transactions not going through because of discoordination between block.timestamp and sender's local timeHowever this is not an issue by adding require(_startTimestamp > uint40(block.timestamp)) since this will revert
transaction if _startTimestamp is less than block.timestamp so administrators can simply try again with correct time.
On the other hand, it is more dangerous to not include this check because transaction will simply succeed even though
_startTimestamp is set to past which means that there is a chance of administrators not noticing this.
Proof of Concept
- Admin creates new vesting schedule using
createClaimfunction. However admin mistakenly set_startTimestampand_endTimestampin the past. - Since there is no check of
require(_startTimestamp > uint40(block.timestamp)), this transaction is valid and claim is created. - Vesting recipients calls the
withdrawfunction and receive entire vest amount.
Recommended Mitigation Steps
Add following check in VTVLVesting.sol:_createClaimUnchecked function.
require(_startTimestamp > uint40(block.timestamp), "INVALID_START_TIMESTAMP")Going to use this issue for encompassing a few different reports that all revolve around adding some better validation around timestamps. These include a few different potential fixes that the sponsor can review, but ultimately point to the same underlying issues.
lawrencehui (VTVL) acknowledged, but disagreed with severity and commented:
As described in the documentation, this back dated (
startTimestamp < block.timestamp) feature is indeed intended as there are many real life cases that founders want to reward their employees in the way the vesting period starts well before Token Generation Event (TGE).We appreciate wardens’ feedback on additional checking (both start and end time) and in our actual application, we would include multiple layer of checking / approval processes in front and backend before the transaction signing happens and therefore the risk is low in our opinion.
[M-08] Two address tokens can be withdrawn by the admin even if they are vested
Submitted by CertoraInc, also found by 0xhunter, datapunk, dipp, Lambda, and wuwe1
Two address tokens exist in the blockchain. For example, Synthetix’s ProxyERC20 contract is such a token which exists in many forms (sUSD, sBTC…). Tokens as such can be vested, but the admin can withdraw them even if they are vested by providing the other address to the withdrawOtherToken function. The only check in this function is that _otherTokenAddress != tokenAddress, which is irrelevant in the case of two address tokens.
This can make the admin be able to withdraw the vested funds and break the system, because the balance of the contract can be less than the vested amount.
Proof of Concept
- The
VTVLVestingis deployed with thesUSDcontract, using its main (proxy) address -0x57Ab1ec28D129707052df4dF418D58a2D46d5f51. - A claim is created for Alice, vesting 1000 sUSD in linear vesting. Assuming this is the only claim currently, the balance of the contract is 1000 sUSD and the value of
numTokensReservedForVestingis1000e18. - The admin calls the
withdrawOtherTokenfor 1000e18 sUSD, providing its second address -0x57Ab1ec28D129707052df4dF418D58a2D46d5f51. The value ofnumTokensReservedForVestingis still1000e18, but the balance of the contract is now 0 sUSD. - Alice waits for her vest to end, calls the withdraw function, but the function reverts on the call to
safeTransfer()because there is insufficient balance of sUSD. Alice can’t receive her funds.
Recommended Mitigation Steps
Replace the address check with a balance check - record the vesting token balance of the contract before and after the transfer and assert that they are equal.
0xean (judge) decreased severity to Medium and commented:
Downgrading to Medium. The fix is a good idea, but this is a pretty rare token implementation and definitely qualifies as an external factor.
lawrencehui (VTVL) acknowledged and commented:
Yes, agreed with @0xean that this is very rare and appreciate warden’s suggestion on the fix on balance checking.
[M-09] _releaseIntervalSecs is not validated
Submitted by sorrynotsorry
VTVLVesting.sol has _createClaimUnchecked function to create the claims internally while validating parameters with the users’ allocations.
However, _releaseIntervalSecs is not validated comparing to user’s _linearVestAmount and _startTimestamp _endTimestamp.
Theoretically, _linearVestAmount should be equal to ((_endTimestamp - _startTimestamp) * _releaseIntervalSecs) so the _releaseIntervalSecs = _linearVestAmount / ((_endTimestamp - _startTimestamp).
But this check was never done.
If the _releaseIntervalSecs is validated either to a higher or to a lower amount, it will create unfair distributions amongst the users during withdrawals due to being higher/lower than it should be. And also it may end up with the last withdrawals can be reverted due to the calculation board not matching.
Proof of Concept
function _createClaimUnchecked(
address _recipient,
uint40 _startTimestamp,
uint40 _endTimestamp,
uint40 _cliffReleaseTimestamp,
uint40 _releaseIntervalSecs,
uint112 _linearVestAmount,
uint112 _cliffAmount
) private hasNoClaim(_recipient) {
require(_recipient != address(0), "INVALID_ADDRESS");
require(_linearVestAmount + _cliffAmount > 0, "INVALID_VESTED_AMOUNT"); // Actually only one of linearvested/cliff amount must be 0, not necessarily both
require(_startTimestamp > 0, "INVALID_START_TIMESTAMP");
// Do we need to check whether _startTimestamp is greater than the current block.timestamp?
// Or do we allow schedules that started in the past?
// -> Conclusion: we want to allow this, for founders that might have forgotten to add some users, or to avoid issues with transactions not going through because of discoordination between block.timestamp and sender's local time
// require(_endTimestamp > 0, "_endTimestamp must be valid"); // not necessary because of the next condition (transitively)
require(_startTimestamp < _endTimestamp, "INVALID_END_TIMESTAMP"); // _endTimestamp must be after _startTimestamp
require(_releaseIntervalSecs > 0, "INVALID_RELEASE_INTERVAL");
require((_endTimestamp - _startTimestamp) % _releaseIntervalSecs == 0, "INVALID_INTERVAL_LENGTH");
// Potential TODO: sanity check, if _linearVestAmount == 0, should we perhaps force that start and end ts are the same?
// No point in allowing cliff TS without the cliff amount or vice versa.
// Both or neither of _cliffReleaseTimestamp and _cliffAmount must be set. If cliff is set, _cliffReleaseTimestamp must be before or at the _startTimestamp
require(
(
_cliffReleaseTimestamp > 0 &&
_cliffAmount > 0 &&
_cliffReleaseTimestamp <= _startTimestamp
) || (
_cliffReleaseTimestamp == 0 &&
_cliffAmount == 0
), "INVALID_CLIFF");
Claim memory _claim = Claim({
startTimestamp: _startTimestamp,
endTimestamp: _endTimestamp,
cliffReleaseTimestamp: _cliffReleaseTimestamp,
releaseIntervalSecs: _releaseIntervalSecs,
cliffAmount: _cliffAmount,
linearVestAmount: _linearVestAmount,
amountWithdrawn: 0,
isActive: true
});
// Our total allocation is simply the full sum of the two amounts, _cliffAmount + _linearVestAmount
// Not necessary to use the more complex logic from _baseVestedAmount
uint112 allocatedAmount = _cliffAmount + _linearVestAmount;
// Still no effects up to this point (and tokenAddress is selected by contract deployer and is immutable), so no reentrancy risk
require(tokenAddress.balanceOf(address(this)) >= numTokensReservedForVesting + allocatedAmount, "INSUFFICIENT_BALANCE");
// Done with checks
// Effects limited to lines below
claims[_recipient] = _claim; // store the claim
numTokensReservedForVesting += allocatedAmount; // track the allocated amount
vestingRecipients.push(_recipient); // add the vesting recipient to the list
emit ClaimCreated(_recipient, _claim); // let everyone know
}Recommended Mitigation Steps
The _releaseIntervalSecs should be validated comparing to user’s _linearVestAmount and _startTimestamp _endTimestamp.
0xean (judge) decreased severity to Medium and commented:
This is fair, but due to it being behind only admin functionality and coming down to input sanitization, going to downgrade to Medium.
lawrencehui (VTVL) acknowledged, but disagreed with severity and commented:
I agree with @0xean that the risk in this case is low given the onlyAdmin modifier and the input will be validated from the frontend anyway. Appreciate the finding and we will take consideration of adding additional checking of
_releaseIntervalSecs.
[M-10] Reentrancy may allow an admin to steal funds
Submitted by Czar102, also found by 0xSmartContract, csanuragjain, hansfriese, Lambda, Respx, and sashik_eth
If the token is reentrant, an admin can steal all tokens locked in the VTVLVesting contract while having active locks.
In other words, due to this exploit possibility, the contract may be insolvent with respect to active vestings. Note that revoking claim doesn’t break this invariant since the vesting is closed in that case.
Proof of Concept
The reentrancy in the vested token can be used by an admin if the execution can be hijacked before the balance change occurs.
/**
@notice Admin withdrawal of the unallocated tokens.
@param _amountRequested - the amount that we want to withdraw
*/
function withdrawAdmin(uint112 _amountRequested) public onlyAdmin {
// Allow the owner to withdraw any balance not currently tied up in contracts.
uint256 amountRemaining = tokenAddress.balanceOf(address(this)) - numTokensReservedForVesting;
require(amountRemaining >= _amountRequested, "INSUFFICIENT_BALANCE");
// Actually withdraw the tokens
// Reentrancy note - this operation doesn't touch any of the internal vars, simply transfers
// Also following Checks-effects-interactions pattern
tokenAddress.safeTransfer(_msgSender(), _amountRequested);
// Let the withdrawal known to everyone
emit AdminWithdrawn(_msgSender(), _amountRequested);
}Let’s consider function withdrawAdmin. Firstly, the balance is checked and then if there is enough token surplus to withdraw, the withdrawal is allowed. The surplus is based on two values: numTokensReservedForVesting which isn’t changed by this function and the balance of the contract.
If the owner hijacks the execution before the balance change in the token transfer (which is possible in, for example, ERC777), an admin can call this function again and it will allow for an invocation of another transfer since the token balance hasn’t changed yet.
For example, if there is $1m in vestings in the contract, an admin can send $100k to it in tokens and recursively invoke withdrawalAdmin with the amount of $100k eleven times so that the whole contract balance will be drained.
Recommended Mitigation Steps
Add ReentrancyGuard’s nonReentrant to the withdrawAdmin function.
This would require a number of assumptions to be the case including a malicious admin which the sponsors called out of scope. Because it is obviously not intended functionality, I am going to leave as Medium pending sponsor review. I think the non-reentrant modifier is worth adding.
lawrencehui (VTVL) acknowledged and commented:
Will consider adding ReentrancyGuard as suggested.
Low Risk and Non-Critical Issues
For this contest, 135 reports were submitted by wardens detailing low risk and non-critical issues. The report highlighted below by AkshaySrivastav received the top score from the judge.
The following wardens also submitted reports: rbserver, IllIllI, 0xNazgul, 0x1f8b, lukris02, rotcivegaf, ajtra, cryptostellar5, 0xSmartContract, Deivitto, Bahurum, brgltd, Diana, c3phas, chatch, ladboy233, supernova, RockingMiles, Waze, tnevler, a12jmx, Funen, pcarranzav, 0x4non, KIntern_NA, __141345__, delfin454000, 0x5rings, Aymen0909, ubermensch, Rolezn, leosathya, 0xf15ers, gogo, CodingNameKiki, V_B, aysha, seyni, Dravee, ChristianKuri, JLevick, CertoraInc, JohnnyTime, Bnke0x0, Lambda, Respx, RaymondFam, rajatbeladiya, ikbkln, neumo, TomJ, 0xA5DF, 0xSky, Aeros, sorrynotsorry, async, prasantgupta52, 0xDecorativePineapple, rvierdiiev, sach1r0, ElKu, slowmoses, neko_nyaa, Tomo, bin2chen, innertia, yongskiws, ignacio, djxploit, JohnSmith, got_targ, joestakey, csanuragjain, rokinot, cryptphi, ayeslick, romand, peanuts, RustyRabbit, 0xbepresent, hansfriese, Chom, berndartmueller, dic0de, peritoflores, zzzitron, cccz, obront, reassor, bobirichman, sikorico, Margaret, datapunk, karanctf, fatherOfBlocks, 0xmatt, nalus, eighty, ret2basic, Ruhum, Sm4rty, Rohan16, pedr02b2, ReyAdmirado, indijanc, SooYa, pedroais, d3e4, ak1, zzykxx, erictee, oyc_109, ch13fd357r0y3r, millersplanet, martin, 2997ms, B2, tibthecat, OptimismSec, exd0tpy, medikko, peiw, JC, StevenL, durianSausage, 0v3rf10w, 0x040, natzuu, Yiko, carrotsmuggler, 0x85102, MasterCookie, bulej93, and Diraco.
[01]
The setAdmin() function in AccessProtected.sol can be used to revoke all admins. This could be a feature to completely renounce ownership of the contract after all claims are set or could be a bug in which one admin either intentionally or unintentionally removes all admin (or all other admins except himself).
[02]
Line 161 in VTVLVesting._baseVestedAmount() function should not get executed when cliffAmount is 0. In the case of no cliff amount, i.e. where cliffReleaseTimestamp and cliffAmount are both set as 0, the program execution should not enter the if block.
160 if(_referenceTs >= _claim.cliffReleaseTimestamp) {
161 vestAmt += _claim.cliffAmount;
162 }[03]
Solidity pragma versioning should be upgraded to latest available version. Currently the solidity version in contracts is ^0.8.14 which was found to possess some bugs.
[04]
Solidity pragma versioning should be exactly same in all contracts. Currently some contracts use ^0.8.14 but some are fixed to 0.8.14.
[05]
No need to re-inherit Context contract in VTVLVesting smart contract as Context is already inherited by AccessProtected contract.
[06]
Ownable smart contract is unnecessarily imported in AccessProtected.sol while it is never used. Unnecessary imports decreases the readability of smart contract code.
[07]
Unnecessary imports are also present in VTVLVesting.sol. The compilation works completely fine with just importing SafeERC20.sol and AccessProtected.sol.
[08]
AccessProtected - contract docs do not match implementaion. The implementation only has multiple equal rights admins and no owner field is present while the docs states something else.
7 /**
8 @title Access Limiter to multiple owner-specified accounts.
9 @dev Exposes the onlyAdmin modifier, which will revert (ADMIN_ACCESS_REQUIRED) if the caller is not the owner nor the admin.
10 */[09]
VariableSupplyERC20Token.constructor() has an empty @dev tag.
[10]
VariableSupplyERC20Token contract mentions an incorrect comment
48 // We can't really have burn, because that could make our vesting contract not work.
49 // Example: if the user can burn tokens already assigned to vesting schedules, it could be unable to pay its obligations.Token can be made burnable in which users can be allowed to burn their own tokens.
[11]
Line 159 in VTVLVesting._baseVestedAmount() contains a misleading comment
159 // We don't check here that cliffReleaseTimestamp is after the startTimestamp
160 if(_referenceTs >= _claim.cliffReleaseTimestamp) {
161 vestAmt += _claim.cliffAmount;
162 }cliffReleaseTimestamp can never be after startTimestamp as per the require statements of _createClaimUnchecked().
[12]
As per the implementation of vesting contract, Line 21 in VTVLVesting.sol should mention greater than or equal instead of just greater than.
21 /// @dev Our balance of the token must always be greater than this amount.[13]
VTVLVesting.ClaimCreated and VTVLVesting.ClaimRevoked events should also log the admin’s address so it can be easily queried which admin created and revoked the claim.
[14]
In VTVLVesting contract, before revoking a claim the contract should transfer all the pending/partially vested rewards. Otherwise the entire vesting amount will get revoked.
It is at the discretion of protocol development team to decide whether the current behaviour is intended or not.[15]
At Line 82 of VTVLVesting.constructor(), a better check would be to do _tokenAddress.totalSupply(). As this will also ensure that the input address in indeed a token’s address and perform the zero address check as well.
82 require(address(_tokenAddress) != address(0), "INVALID_ADDRESS");[16]
The tokenAddress state variable of VTVLVesting should be renamed to token as this variable represents an IERC20 interface rather that just an address. Renaming it to token aligns better with its usage.
17 IERC20 public immutable tokenAddress;[17]
There should be a factory contract for VTVLVesting contract which can keep track of all vesting contracts deployed by different founders. The Factory contract aligns better with the business usecase of VTVL protocol owners.
From the spec, "The core function of VTVL is to allow users to generate and deploy token vesting smart contracts through our platform."[18]
In VariableSupplyERC20Token.mint() function, non-zero input validation check should be done similar to FullPremintERC20Token.constructor().
[19]
In all solidity files, license keyword should be mentioned as // SPDX-License-Identifier: UNLICENSED.
[20]
All the actors interacting with a VTVLVesting contract need to fully trust all of its admins. Any one of the potentially infinite admins of VTVLVesting contract has the power to (either intentionally or unintentionally): * revoke claims of all recipients and withdraw all tokens, resulting in a rugpull attack. * give or take back the admin rights to or from any ethereum address. * withdraw any other ERC20 token from the vesting contract.
Gas Optimizations
For this contest, 141 reports were submitted by wardens detailing gas optimizations. The report highlighted below by IllIllI received the top score from the judge.
The following wardens also submitted reports: Aymen0909, pfapostol, c3phas, JLevick, Deivitto, gogo, CertoraInc, JC, Bnke0x0, oyc_109, durianSausage, __141345__, rotcivegaf, 0xSmartContract, ajtra, Sm4rty, cryptostellar5, Diana, JohnSmith, Tomo, zishansami, martin, ch0bu, SnowMan, prasantgupta52, erictee, millersplanet, djxploit, Rohan16, 0x1f8b, RaymondFam, 0x4non, rbserver, Rolezn, TomJ, brgltd, 0xNazgul, Saintcode_, karanctf, medikko, ret2basic, 0xsam, ReyAdmirado, seyni, gianganhnguyen, Ruhum, carrotsmuggler, slowmoses, WilliamAmbrozic, B2, peiw, 0x040, leosathya, delfin454000, Tomio, samruna, lukris02, aysha, yaemsobak, Junnon, imare, eighty, 0xA5DF, ladboy233, emrekocak, tnevler, pauliax, ikbkln, neko_nyaa, jag, Tadashi, Atarpara, tgolding55, 0xbepresent, Ocean_Sky, peanuts, caventa, RockingMiles, supernova, SooYa, beardofginger, natzuu, pedroais, bobirichman, dharma09, DimitarDimitrov, sach1r0, Waze, ignacio, async, tibthecat, OptimismSec, AkshaySrivastav, malinariy, lucacez, ChristianKuri, Chom, Funen, d3e4, subtle77, fatherOfBlocks, 0xDanielC, indijanc, ak1, got_targ, mics, Lambda, KIntern_NA, w0Lfrum, hxzy, Amithuddar, V_B, Tagir2003, 0xc0ffEE, Respx, MasterCookie, Satyam_Sharma, Noah3o6, rokinot, nalus, jpserrat, CodingNameKiki, Matin, rvierdiiev, adriro, StevenL, bulej93, 2997ms, Diraco, csanuragjain, Sta1400, 0v3rf10w, 0x85102, mrpathfindr, exd0tpy, cryptphi, a12jmx, francoHacker, m9800, and Yiko.
Summary
| Issue | Instances | Total Gas Saved | |
|---|---|---|---|
| [G‑01] | Save gas by not requring non-zero interval if no linear amount | 1 | 17100 |
| [G‑02] | Results of calls to _msgSender() not cached |
4 | 64 |
| [G‑03] | Using calldata instead of memory for read-only arguments in external functions saves gas |
7 | 840 |
| [G‑04] | State variables should be cached in stack variables rather than re-reading them from storage | 1 | 97 |
| [G‑05] | <x> += <y> costs more gas than <x> = <x> + <y> for state variables |
4 | 452 |
| [G‑06] | Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement |
4 | 340 |
| [G‑07] | ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops |
1 | 60 |
| [G‑08] | Optimize names to save gas | 3 | 66 |
| [G‑09] | Using bools for storage incurs overhead |
1 | 20000 |
| [G‑10] | ++i costs less gas than i++, especially when it’s used in for-loops (--i/i-- too) |
1 | 10 |
| [G‑11] | Splitting require() statements that use && saves gas |
1 | 3 |
| [G‑12] | Don’t compare boolean expressions to boolean literals | 1 | 9 |
| [G‑13] | Use custom errors rather than revert()/require() strings to save gas |
24 | - |
| [G‑14] | Functions guaranteed to revert when called by normal users can be marked payable |
7 | 147 |
| [G‑15] | Don’t use _msgSender() if not supporting EIP-2771 |
13 | 208 |
Total: 73 instances over 15 issues with 39396 gas saved
Gas totals use lower bounds of ranges and count two iterations of each for-loop. All values above are runtime, not deployment, values.
[G‑01] Save gas by not requring non-zero interval if no linear amount
If there is no linear amount, a Gsset for the claim’s interval can be converted to a Gsreset, saving 17100 gas.
There is 1 instance of this issue:
File: /contracts/VTVLVesting.sol
263 require(_releaseIntervalSecs > 0, "INVALID_RELEASE_INTERVAL");
264: require((_endTimestamp - _startTimestamp) % _releaseIntervalSecs == 0, "INVALID_INTERVAL_LENGTH");[G‑02] Results of calls to _msgSender() not cached
Saves at least 16 gas per call skipped.
There are 4 instances of this issue:
File: /contracts/VTVLVesting.sol
371: uint112 allowance = vestedAmount(_msgSender(), uint40(block.timestamp));
388: tokenAddress.safeTransfer(_msgSender(), amountRemaining);
391: emit Claimed(_msgSender(), amountRemaining);
410: emit AdminWithdrawn(_msgSender(), _amountRequested);[G‑03] Using calldata instead of memory for read-only arguments in external functions saves gas
When a function with a memory array is called externally, the abi.decode() step has to use a for-loop to copy each index of the calldata to the memory index. Each iteration of this for-loop costs at least 60 gas (i.e. 60 * <mem_array>.length). Using calldata directly, obliviates the need for such a loop in the contract code and runtime execution. Note that even if an interface defines a function as having memory arguments, it’s still valid for implementation contracs to use calldata arguments instead.
If the array is passed to an internal function which passes the array to another internal function where the array is modified and therefore memory is used in the external call, it’s still more gass-efficient to use calldata when the external function uses modifiers, since the modifiers may prevent the internal functions from being called. Structs have the same overhead as an array of length one.
Note that I’ve also flagged instances where the function is public but can be marked as external since it’s not called by the contract, and cases where a constructor is involved.
There are 7 instances of this issue:
File: contracts/VTVLVesting.sol
/// @audit _recipients
/// @audit _startTimestamps
/// @audit _endTimestamps
/// @audit _cliffReleaseTimestamps
/// @audit _releaseIntervalsSecs
/// @audit _linearVestAmounts
/// @audit _cliffAmounts
333 function createClaimsBatch(
334 address[] memory _recipients,
335 uint40[] memory _startTimestamps,
336 uint40[] memory _endTimestamps,
337 uint40[] memory _cliffReleaseTimestamps,
338 uint40[] memory _releaseIntervalsSecs,
339 uint112[] memory _linearVestAmounts,
340 uint112[] memory _cliffAmounts)
341: external onlyAdmin {[G‑04] State variables should be cached in stack variables rather than re-reading them from storage
The instances below point to the second+ access of a state variable within a function. Caching of a state variable replace each Gwarmaccess (100 gas) with a much cheaper stack read. Other less obvious fixes/optimizations include having local memory caches of state variable structs, or having local caches of state variable contracts/addresses.
There is 1 instance of this issue:
File: contracts/token/VariableSupplyERC20Token.sol
/// @audit mintableSupply on line 40
41: require(amount <= mintableSupply, "INVALID_AMOUNT");VariableSupplyERC20Token.sol#L41
[G‑05] <x> += <y> costs more gas than <x> = <x> + <y> for state variables
Using the addition operator instead of plus-equals saves 113 gas.
There are 4 instances of this issue:
File: contracts/token/VariableSupplyERC20Token.sol
43: mintableSupply -= amount;VariableSupplyERC20Token.sol#L43
File: contracts/VTVLVesting.sol
301: numTokensReservedForVesting += allocatedAmount; // track the allocated amount
383: numTokensReservedForVesting -= amountRemaining;
433: numTokensReservedForVesting -= amountRemaining; // Reduces the allocation[G‑06] Add unchecked {} for subtractions where the operands cannot underflow because of a previous require() or if-statement
require(a <= b); x = b - a => require(a <= b); unchecked { x = b - a }
There are 4 instances of this issue:
File: contracts/VTVLVesting.sol
/// @audit require() on line 262
264: require((_endTimestamp - _startTimestamp) % _releaseIntervalSecs == 0, "INVALID_INTERVAL_LENGTH");
/// @audit require() on line 374
377: uint112 amountRemaining = allowance - usrClaim.amountWithdrawn;
/// @audit require() on line 426
429: uint112 amountRemaining = finalVestAmt - _claim.amountWithdrawn;
/// @audit if-condition on line 166
167: uint40 currentVestingDurationSecs = _referenceTs - _claim.startTimestamp; // How long since the start[G‑07] ++i/i++ should be unchecked{++i}/unchecked{i++} when it is not possible for them to overflow, as is the case when used in for- and while-loops
The unchecked keyword is new in solidity version 0.8.0, so this only applies to that version or higher, which these instances are. This saves 30-40 gas per loop.
There is 1 instance of this issue:
File: contracts/VTVLVesting.sol
353: for (uint256 i = 0; i < length; i++) {[G‑08] Optimize names to save gas
public/external function names and public member variable names can be optimized to save gas. See this link for an example of how it works. Below are the interfaces/abstract contracts that can be optimized so that the most frequently-called functions use the least amount of gas possible during method lookup. Method IDs that have two leading zero bytes can save 128 gas each during deployment, and renaming functions to have lower method IDs will save 22 gas per call, per sorted position shifted.
There are 3 instances of this issue:
File: contracts/AccessProtected.sol
/// @audit isAdmin(), setAdmin()
11: abstract contract AccessProtected is Context {File: contracts/token/VariableSupplyERC20Token.sol
/// @audit mint()
10: contract VariableSupplyERC20Token is ERC20, AccessProtected {VariableSupplyERC20Token.sol#L10
File: contracts/VTVLVesting.sol
/// @audit getClaim(), vestedAmount(), finalVestedAmount(), claimableAmount(), allVestingRecipients(), numVestingRecipients(), createClaim(), createClaimsBatch(), withdraw(), withdrawAdmin(), revokeClaim(), withdrawOtherToken()
11: contract VTVLVesting is Context, AccessProtected {[G‑09] Using bools for storage incurs overhead
// Booleans are more expensive than uint256 or any type that takes up a full
// word because each write operation emits an extra SLOAD to first read the
// slot's contents, replace the bits taken up by the boolean, and then write
// back. This is the compiler's defense against contract upgrades and
// pointer aliasing, and it cannot be disabled.OpenZeppelin/ReentrancyGuard.sol#L23-L27
Use uint256(1) and uint256(2) for true/false to avoid a Gwarmaccess (100 gas) for the extra SLOAD, and to avoid Gsset (20000 gas) when changing from false to true, after having been true in the past.
There is 1 instance of this issue:
File: contracts/AccessProtected.sol
12: mapping(address => bool) private _admins; // user address => admin? mapping[G‑10] ++i costs less gas than i++, especially when it’s used in for-loops (--i/i-- too)
Saves 5 gas per loop.
There is 1 instance of this issue:
File: contracts/VTVLVesting.sol
353: for (uint256 i = 0; i < length; i++) {[G‑11] Splitting require() statements that use && saves gas
See this issue which describes the fact that there is a larger deployment gas cost, but with enough runtime calls, the change ends up being cheaper by 3 gas.
There is 1 instance of this issue:
File: contracts/VTVLVesting.sol
344 require(_startTimestamps.length == length &&
345 _endTimestamps.length == length &&
346 _cliffReleaseTimestamps.length == length &&
347 _releaseIntervalsSecs.length == length &&
348 _linearVestAmounts.length == length &&
349 _cliffAmounts.length == length,
350 "ARRAY_LENGTH_MISMATCH"
351: );[G‑12] Don’t compare boolean expressions to boolean literals
if (<x> == true) => if (<x>), if (<x> == false) => if (!<x>)
There is 1 instance of this issue:
File: contracts/VTVLVesting.sol
111: require(_claim.isActive == true, "NO_ACTIVE_CLAIM");[G‑13] Use custom errors rather than revert()/require() strings to save gas
Custom errors are available from solidity version 0.8.4. Custom errors save ~50 gas each time they’re hit by avoiding having to allocate and store the revert string. Not defining the strings also save deployment gas.
There are 24 instances of this issue:
File: contracts/AccessProtected.sol
25: require(_admins[_msgSender()], "ADMIN_ACCESS_REQUIRED");
40: require(admin != address(0), "INVALID_ADDRESS");File: contracts/token/FullPremintERC20Token.sol
11: require(supply_ > 0, "NO_ZERO_MINT");File: contracts/token/VariableSupplyERC20Token.sol
27: require(initialSupply_ > 0 || maxSupply_ > 0, "INVALID_AMOUNT");
37: require(account != address(0), "INVALID_ADDRESS");
41: require(amount <= mintableSupply, "INVALID_AMOUNT");VariableSupplyERC20Token.sol#L27
File: contracts/VTVLVesting.sol
82: require(address(_tokenAddress) != address(0), "INVALID_ADDRESS");
107: require(_claim.startTimestamp > 0, "NO_ACTIVE_CLAIM");
111: require(_claim.isActive == true, "NO_ACTIVE_CLAIM");
129: require(_claim.startTimestamp == 0, "CLAIM_ALREADY_EXISTS");
255: require(_recipient != address(0), "INVALID_ADDRESS");
256: require(_linearVestAmount + _cliffAmount > 0, "INVALID_VESTED_AMOUNT"); // Actually only one of linearvested/cliff amount must be 0, not necessarily both
257: require(_startTimestamp > 0, "INVALID_START_TIMESTAMP");
262: require(_startTimestamp < _endTimestamp, "INVALID_END_TIMESTAMP"); // _endTimestamp must be after _startTimestamp
263: require(_releaseIntervalSecs > 0, "INVALID_RELEASE_INTERVAL");
264: require((_endTimestamp - _startTimestamp) % _releaseIntervalSecs == 0, "INVALID_INTERVAL_LENGTH");
270 require(
271 (
272 _cliffReleaseTimestamp > 0 &&
273 _cliffAmount > 0 &&
274 _cliffReleaseTimestamp <= _startTimestamp
275 ) || (
276 _cliffReleaseTimestamp == 0 &&
277 _cliffAmount == 0
278: ), "INVALID_CLIFF");
295: require(tokenAddress.balanceOf(address(this)) >= numTokensReservedForVesting + allocatedAmount, "INSUFFICIENT_BALANCE");
344 require(_startTimestamps.length == length &&
345 _endTimestamps.length == length &&
346 _cliffReleaseTimestamps.length == length &&
347 _releaseIntervalsSecs.length == length &&
348 _linearVestAmounts.length == length &&
349 _cliffAmounts.length == length,
350 "ARRAY_LENGTH_MISMATCH"
351: );
374: require(allowance > usrClaim.amountWithdrawn, "NOTHING_TO_WITHDRAW");
402: require(amountRemaining >= _amountRequested, "INSUFFICIENT_BALANCE");
426: require( _claim.amountWithdrawn < finalVestAmt, "NO_UNVESTED_AMOUNT");
447: require(_otherTokenAddress != tokenAddress, "INVALID_TOKEN"); // tokenAddress address is already sure to be nonzero due to constructor
449: require(bal > 0, "INSUFFICIENT_BALANCE");[G‑14] Functions guaranteed to revert when called by normal users can be marked payable
If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function. Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided. The extra opcodes avoided are CALLVALUE(2),DUP1(3),ISZERO(3),PUSH2(3),JUMPI(10),PUSH1(3),DUP1(3),REVERT(0),JUMPDEST(1),POP(2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost.
There are 7 instances of this issue:
File: contracts/AccessProtected.sol
39: function setAdmin(address admin, bool isEnabled) public onlyAdmin {File: contracts/token/VariableSupplyERC20Token.sol
36: function mint(address account, uint256 amount) public onlyAdmin {VariableSupplyERC20Token.sol#L36
File: contracts/VTVLVesting.sol
317 function createClaim(
318 address _recipient,
319 uint40 _startTimestamp,
320 uint40 _endTimestamp,
321 uint40 _cliffReleaseTimestamp,
322 uint40 _releaseIntervalSecs,
323 uint112 _linearVestAmount,
324 uint112 _cliffAmount
325: ) external onlyAdmin {
333 function createClaimsBatch(
334 address[] memory _recipients,
335 uint40[] memory _startTimestamps,
336 uint40[] memory _endTimestamps,
337 uint40[] memory _cliffReleaseTimestamps,
338 uint40[] memory _releaseIntervalsSecs,
339 uint112[] memory _linearVestAmounts,
340 uint112[] memory _cliffAmounts)
341: external onlyAdmin {
398: function withdrawAdmin(uint112 _amountRequested) public onlyAdmin {
418: function revokeClaim(address _recipient) external onlyAdmin hasActiveClaim(_recipient) {
446: function withdrawOtherToken(IERC20 _otherTokenAddress) external onlyAdmin {[G‑15] Don’t use _msgSender() if not supporting EIP-2771
Use msg.sender if the code does not implement EIP-2771 trusted forwarder support.
There are 13 instances of this issue:
File: contracts/AccessProtected.sol
17: _admins[_msgSender()] = true;
18: emit AdminAccessSet(_msgSender(), true);
25: require(_admins[_msgSender()], "ADMIN_ACCESS_REQUIRED");File: contracts/token/FullPremintERC20Token.sol
12: _mint(_msgSender(), supply_);File: contracts/token/VariableSupplyERC20Token.sol
32: mint(_msgSender(), initialSupply_);VariableSupplyERC20Token.sol#L32
File: contracts/VTVLVesting.sol
367: Claim storage usrClaim = claims[_msgSender()];
371: uint112 allowance = vestedAmount(_msgSender(), uint40(block.timestamp));
388: tokenAddress.safeTransfer(_msgSender(), amountRemaining);
391: emit Claimed(_msgSender(), amountRemaining);
364: function withdraw() hasActiveClaim(_msgSender()) external {
407: tokenAddress.safeTransfer(_msgSender(), _amountRequested);
410: emit AdminWithdrawn(_msgSender(), _amountRequested);
450: _otherTokenAddress.safeTransfer(_msgSender(), bal);Disclosures
C4 is an open organization governed by participants in the community.
C4 Contests incentivize the discovery of exploits, vulnerabilities, and bugs in smart contracts. Security researchers are rewarded at an increasing rate for finding higher-risk issues. Contest submissions are judged by a knowledgeable security researcher and solidity developer and disclosed to sponsoring developers. C4 does not conduct formal verification regarding the provided code but instead provides final verification.
C4 does not provide any guarantee or warranty regarding the security of this project. All smart contract software should be used at the sole risk and responsibility of users.