VTVL contest - Respx's results

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L183-L187 https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L198

Vulnerability details

Description

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 VTVLVesting and attempts to use _baseVestedAmount() (which is internal and so can be used by inheriting contracts). The inheriting contract might apportion rewards based on historical usage.
• VTVL itself might wish to inherit from VTVLVesting in 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 () => {

Tools Used

Manual Inspection

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, but it is required to provide monotonicity.

Lines of code

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L398-L411

Vulnerability details

Impact

This exploit allows an admin to empty out all reward tokens without triggering any revoke events or updating any claims to revoked. For this scenario to be dangerous, the admin would need to set up arrangements whereby payments were made by other contracts based on the existence of claims in the VTVLVesting contract, and on those claims not being revoked. It is conceivable, however, that if VTVL became established wth a decent reputation, than a malicious actor might well be able to convince others to trust the VTVL vesting contract as a trigger condition for payments. So long as those other contracts do not check _claim.amountWithdrawn, but rely on the timestamps, values and isActive flag, they would be tricked by this reentrancy exploit. This would cause significant damage to the tricked users and also to the VTVL project.

Proof of Concept

Reentrancy is possible within withdrawAdmin() by an admin user in very specific circumstances. It requires the token tokenAddress to respond to transfers by passing control before the transfer. If such a hook exists, the reentrancy would work like this:

1. Admin transfers tokens to the contract until the balance is double numTokensReservedForVesting.
2. Admin calls withdrawAdmin() with _amountRequested set to numTokensReservedForVesting.
3. On line 400, amountRemaining will be set to (2xnumTokensReservedForVesting)-numTokensReservedForVesting = numTokensReservedForVesting.
4. On line 407, execution flow passes to the token contract before the tokens are transferred. The token contract calls an attack contract (which the admin has previously granted admin status to).
5. The attack contract calls withdrawAdmin() with _amountRequested set to numTokensReservedForVesting.
6. On line 400, amountRemaining will still be set to numTokensReservedForVesting as the balance of the contract has not yet changed.
7. The transfer on line 407 is executed and the attack contract takes no action when it is called this time.
8. The attack contract receives numTokensReservedForVesting tokens.
9. Execution returns from the attack contract to the original admin call (which it left in point 4).
10. The transfer to admin of numTokensReservedForVesting completes, emptying the VTVLVesting contract of tokenAddress tokens without revoking any claims.

Tools Used

Manual Inspection

Recommended Mitigation Steps

Use a reentrancy guard on withdrawAdmin() or, if that is strongly not desired, implement a deposit function for tokenAddress and have the contract track its own balance internally so that this value can be updated before any transfers.

Overview

Generally speaking, there are a lot of comments, which is good, but the quality of comments needs to be improved. I have tried to highlight as many technical comment errors as I could, but there are some other general issues.

1. The language also needs a few proofreads. There are many sentences like "// Let the withdrawal known to everyone" which could be rewritten to be clearer. Their cumulative effect is to make the code harder to understand. As this project is designed for general use, these rewrites would be especially worthwhile.
2. There are too many comments which imply that the code is unfinished, or design decisions are still unresolved. This implies to a potential user that the code is not finalised and therefore not stable. All design questions should be fully resolved before release (and before audit) and the comments should reflect that.

---

Non Critical

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/token/VariableSupplyERC20Token.sol#L22 Use the actual variable name in this comment, as on line 23.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/token/VariableSupplyERC20Token.sol#L30 Write "function", not "fn" for clarity.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/AccessProtected.sol#L16 The constructor is missing its comment header.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/AccessProtected.sol#L29 isAdmin() is missing its comment header.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L17 https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L83 Note that the variable tokenAddress is actually the token, not its address. The address is address(tokenAddress). Consider renaming this to token.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L121 Whilst logically this is correct, it could be better explained. The modifier only requires one thing, so this could be better expressed.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L159 Remove this comment, or add significant explanation as to why you are mentioning this here (probably better to remove). Also, cliffReleaseTimestamp is not allowed to be after startTimestamp, so it is doubly confusing.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L258-L260 For clarity and confidence, rewrite these comments as a statement or two, not as a discussion with yourself.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L245 As _createClaimUnchecked() consists mostly of checks, consider some alternative names to reflect the fact that it is an internal function without access modifiers: _permissionlessCreateClaim, _privateCreateClaim or just _createClaim

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L365 Remove " - if any" from this comment as hasActiveClaim(_msgSender()) on line 364 guarantees there will be a claim.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L398 For clarity, consider renaming withdrawAdmin to adminWithdraw(). Generally, if the noun is first it is the thing acting. If it is second, it is the thing being acted upon.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L203 As in the summary, this is not the sort of comment that should reach production. It will hurt confidence in the code. If the function should be removed, it should already be gone. If not, this comment certainly should be.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L266 If you are asking for an opinion, I think it would be good to enforce _cliffReleaseTimestamp == _startTimestamp == _endTimestamp in that scenario. Regardless, this issue should be decided and this comment should be removed for confidence and clarity.

---

Low

Technically incorrect comments have been included in this section. Unclear comments are above.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/AccessProtected.sol#L22 The comment refers to an "owner" but there is no such thing in this contract.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/token/VariableSupplyERC20Token.sol#L25-L27 To answer the question in the comment: yes, I think you should allow both to be zero. Your reasons for allowing initialSupply_ == 0 are not affected by your reasons for allowing maxSupply_ == 0, or vice versa. I suggest you remove lines 25-27.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/AccessProtected.sol#L39-L43 Note that it is possible with setAdmin() to remove all admins and leave the contract without admins permanently. This might be desirable for some contracts, but consider whether it is wanted for this project. If not, one possible protection is to keep a counter of the number of admins and refuse to remove an admin if it would cause the counter to go below 1. The counter's limit could also be an immutable variable set in the constructor to allow extra flexibility.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L126 This comment incorrectly states that an existing claim can have its start timestamp modified in createClaim(). Remove the part in brackets to correct it.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L41 This comment is incorrect and contradicts the previous line. It should be removed.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L44 " - released at the cliffReleaseTimestamp" at the end of this comment is incorrect and appears to be accidentally copied from the previous line.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L120 This comment is technically incorrect. They are not opposites as both modifiers will revert if _claim.startTimestamp == 0 and _claim.isActive == false.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L370 Change the comment to "40 bit".

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L245-L304 There is no check that _endTimestamp is not in the past, even _linearVestAmount > 0.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L53 The value of the state variable vestingRecipients seems a little dubious. It will contain all recipients ever, even those with revoked claims or claims that reached their end time long ago. Consider implementing an enumerable mapping library to have a list which contains only the addresses with active claims.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L256 https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L292 allocatedAmount can be caulculated earlier in the function and then used in the test on line 256.

https://github.com/code-423n4/2022-09-vtvl/blob/f68b7f3e61dad0d873b5b5a1e8126b839afeab5f/contracts/VTVLVesting.sol#L374-L377 usrClaim.amountWithdrawn is accessed twice from storage. It can be copied into a memory variable on the first access and then that memory variable used on line 377 to save gas.