Core Contracts
Regnum Aurum Acquisition Corp
HardhatReal World AssetsNFT
77,280 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Users can `lock()` twice on `veRAACToken`, this can leave tokens stuck and it is incorrect

charlescheerful

Summary

When a user locks RAACToken to get veRAAC, they can lock tokens again, and the previous data gets overridden. This behavior breaks the system mechanics as described below.

Vulnerability Details

In veRAACToken::lock(), there is no check to determine if the user has already locked tokens for a given period. This leads to the state of _lockState.locks[msg.sender] being overwritten with new amounts and lock durations.

Proof of Concept

A user can call veRAACToken::lock() multiple times with smaller amounts and shorter durations because there are no checks to prevent it.

Follow along with the comments in the code to understand the flow. You can see the code here:

function lock(uint256 amount, uint256 duration) external nonReentrant whenNotPaused {
@> // 👁️1️⃣🟢 These checks ensure that the amount is not too high and the duration is valid.
if (amount == 0) revert InvalidAmount();
if (amount > MAX_LOCK_AMOUNT) revert AmountExceedsLimit();
if (totalSupply() + amount > MAX_TOTAL_SUPPLY) revert TotalSupplyLimitExceeded();
if (duration < MIN_LOCK_DURATION || duration > MAX_LOCK_DURATION) revert InvalidLockDuration();
​
@> // 👁️2️⃣🟢 No checks exist to prevent overriding existing locks. The second lock still executes.
​
raacToken.safeTransferFrom(msg.sender, address(this), amount);
uint256 unlockTime = block.timestamp + duration;
​
@> // 👁️3️⃣🟢 `createLock` has no checks to prevent multiple locks.
_lockState.createLock(msg.sender, amount, duration);
​
@> // 👁️4️⃣🟢 `_updateBoostState` does not check for existing locks either.
_updateBoostState(msg.sender, amount);
​
@> // 👁️5️⃣🟢 The rest of the funciton just claculates voting power amounts and writes
@> // 👁️🟢 to storage. But no checks are applied to see if there is already a lock created.
// Calculate initial voting power
(int128 bias, int128 slope) = _votingState.calculateAndUpdatePower(
msg.sender,
amount,
unlockTime
);
​
// Update checkpoints
uint256 newPower = uint256(uint128(bias));
_checkpointState.writeCheckpoint(msg.sender, newPower);
​
// Mint veTokens
_mint(msg.sender, newPower);
​
emit LockCreated(msg.sender, amount, unlockTime);
}

  • createLock(): No check for an existing lock. See here.

  • _updateBoostState(): No check for an existing lock. See here.

  • calculateAndUpdatePower(): Only validates duration and amount, but not if a lock already exists. See here.

Impact

  1. Incorrect Code Logic: Other functions that manipulate locks do check for existing locks and act accordingly. The lock function is inconsistent in this regard.

  2. Token Loss Risk: If a user calls lock() multiple times, some tokens can get stuck in the contract because withdrawal functions read the overwritten amount value to use in the transfer. Neither withdraw() nor emergencyWithdraw() can recover these tokens, as the value has been reset. See withdraw here and emergency withdraw here.

Recommendations

Add a check to prevent a user from locking more tokens if they already have an active lock. Users should use the existing incerase() to do so:

function lock(uint256 amount, uint256 duration) external nonReentrant whenNotPaused {
if (amount == 0) revert InvalidAmount();
if (amount > MAX_LOCK_AMOUNT) revert AmountExceedsLimit();
if (totalSupply() + amount > MAX_TOTAL_SUPPLY) revert TotalSupplyLimitExceeded();
if (duration < MIN_LOCK_DURATION || duration > MAX_LOCK_DURATION) revert InvalidLockDuration();
+ LockManager.Lock memory userLock = _lockState.locks[msg.sender];
+ if (userLock.exists) revert UserAlreadyLocked();
​
raacToken.safeTransferFrom(msg.sender, address(this), amount);
uint256 unlockTime = block.timestamp + duration;
_lockState.createLock(msg.sender, amount, duration);
_updateBoostState(msg.sender, amount);
// Additional logic...
}
Updates

Lead Judging Commences

inallhonesty Lead Judge about 1 month ago
Submission Judgement Published
Validated
Assigned finding tags:

veRAACToken::lock called multiple times, by the same user, leads to loss of funds

Support

FAQs

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