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

Overwriting User Locks in veRAACToken Contract Leading to Loss of Funds

roccomania

https://github.com/Cyfrin/2025-02-raac/blob/main/contracts/core/tokens/veRAACToken.sol#L212-L244
https://github.com/Cyfrin/2025-02-raac/blob/main/contracts/libraries/governance/LockManager.sol#L117-L143

Summary

The veRAACToken contract allows users to lock RAAC tokens to receive voting power. However, the lock function overwrites existing locks when a user creates a new lock, causing the loss of previously locked tokens. This vulnerability arises because the LockManager library does not check for existing locks before creating a new one, and the withdraw function does not account for multiple locks.

Vulnerability Details

Explanation

When a user creates a new lock using the veRAACToken::lock function, the LockManager library's createLock function overwrites the existing lock without transferring the previously locked tokens back to the user. This results in the loss of the user's previously locked tokens, as they remain locked in the contract and are inaccessible.

Root Cause in the Contract Function

  1. Overwriting Existing Locks:

    • The veRAACToken::lock uses the createLock function in the LockManager library which overwrites the existing lock without checking if the user already has a lock:

contract veRAACToken is ERC20, Ownable, ReentrancyGuard, IveRAACToken {
//Rest of the code
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();
​
// Do the transfer first - this will revert with ERC20InsufficientBalance if user doesn't have enough tokens
raacToken.safeTransferFrom(msg.sender, address(this), amount);
// Calculate unlock time
uint256 unlockTime = block.timestamp + duration;
// Create lock position
@> _lockState.createLock(msg.sender, amount, duration); //@audit-issue creating new lock overrides previous lock as seen in LockManager::createLock
_updateBoostState(msg.sender, amount);
​
// 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);
}
//Rest of the code
​
}

LockManager Library

library LockManager {
//Rest of the code
​
function createLock(
LockState storage state,
address user,
uint256 amount,
uint256 duration
) internal returns (uint256 end) {
// ... (validation logic)
@> state.locks[user] = Lock({ // new locks will be overriden here
amount: amount,
end: end,
exists: true
});
// ... (other logic)
}
//Rest of the code
}
​

2.Insufficient Withdraw Logic:

  • The withdraw function deletes the lock state and voting state but does not account for multiple locks or ensure that all locked tokens are returned to the user:

function withdraw() external nonReentrant {
LockManager.Lock memory userLock = _lockState.locks[msg.sender];
// ... (validation logic)
delete _lockState.locks[msg.sender];
delete _votingState.points[msg.sender];
// ... (other logic)
}

Proof of Concept

Scenario Example

  1. User Creates First Lock: A user locks 1000 RAAC tokens for 365 days.

  2. User Creates Second Lock: The user locks an additional 500 RAAC tokens for 365 days, overwriting the first lock.

  3. User Withdraws: The user withdraws after the lock expires but only receives the 500 RAAC tokens from the second lock. The 1000 RAAC tokens from the first lock are lost.

Code

The vulnerability is demonstrated in the following Foundry test suite. Convert to foundry project using the steps highlighted here. Then in the test/ folder create a Test file named veRAACTokenTest.t.sol and paste the test into it. Make sure the imports path are correct and run the test using forge test --mt testWithdrawAfterLockExpires :

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;
​
import "forge-std/Test.sol";
import {veRAACToken} from "contracts/core/tokens/veRAACToken.sol";
import "contracts/interfaces/core/tokens/IveRAACToken.sol";
import "contracts/core/tokens/RAACToken.sol";
​
contract veRAACTokenTest is Test {
veRAACToken public veRAAC;
RAACToken public raacToken;
address public owner;
address[] public users;
​
uint256 public constant MIN_LOCK_DURATION = 365 days;
uint256 public constant MAX_LOCK_DURATION = 1460 days;
uint256 public constant INITIAL_MINT = 1000000 ether;
​
function setUp() public {
owner = address(this);
users.push(address(0x1));
users.push(address(0x2));
users.push(address(0x3));
​
raacToken = new RAACToken(owner, 100, 50);
raacToken.setMinter(owner);
veRAAC = new veRAACToken(address(raacToken));
raacToken.manageWhitelist(address(veRAAC), true);
​
// Setup initial token balances and approvals
for (uint256 i = 0; i < users.length; i++) {
raacToken.mint(users[i], INITIAL_MINT);
vm.prank(users[i]);
raacToken.approve(address(veRAAC), type(uint256).max);
}
}
​
function testWithdrawAfterLockExpires() public {
uint256 amount = 1000 ether;
uint256 amount2 = 500 ether;
uint256 duration = 365 days;
​
uint256 initialUserBal = raacToken.balanceOf(users[0]);
​
//User create first lock
vm.prank(users[0]);
veRAAC.lock(amount, duration);
​
vm.warp(block.timestamp + duration + 1);
​
IveRAACToken.LockPosition memory vRaacPos = veRAAC.getLockPosition(users[0]);
console.log("vRaacPos lock amount", vRaacPos.amount);
console.log("vRaacPos lock end", vRaacPos.end);
console.log("vRaacPos lock power", vRaacPos.power);
​
//User creates new lock
console.log("===============Creating Second Lock======================");
vm.prank(users[0]);
veRAAC.lock(amount2, duration);
​
vm.warp(block.timestamp + duration + 1);
​
IveRAACToken.LockPosition memory vRaacPos2 = veRAAC.getLockPosition(users[0]);
console.log("vRaacPos2 lock amount", vRaacPos2.amount);
console.log("vRaacPos2 lock end", vRaacPos2.end);
console.log("vRaacPos2 lock power", vRaacPos2.power);
​
​
//Asserts that funds wer deducted from user balance correctly
uint256 balanceBefore = raacToken.balanceOf(users[0]);
assertEq(initialUserBal, balanceBefore + (amount + amount2));
​
//User withdraws after lock expires
vm.prank(users[0]);
veRAAC.withdraw();
​
uint256 VeRaacRAACBalAfter = raacToken.balanceOf(address(veRAAC));
uint256 balanceAfter = raacToken.balanceOf(users[0]);
​
//User was not able to et back all his locked funds
assertLt(balanceAfter, initialUserBal);
​
//This prooves that the first lock amount is still locked in the veRAACToken contract and is lost by the owner
assertEq(VeRaacRAACBalAfter, initialUserBal - balanceAfter);
}
​
}
​

Bash Result

╰─ forge test --mt testWithdrawAfterLockExpires -vvv
[⠊] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/foundry/veRAACTokenTest.t.sol:veRAACTokenTest
[PASS] testWithdrawAfterLockExpires() (gas: 503458)
Logs:
vRaacPos lock amount 1000000000000000000000
vRaacPos lock end 31536001
vRaacPos lock power 250000000000000000000
===============Creating Second Lock======================
vRaacPos2 lock amount 500000000000000000000
vRaacPos2 lock end 63072002
vRaacPos2 lock power 375000000000000000000
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.35ms (449.10µs CPU time)
​
Ran 1 test suite in 8.74ms (1.35ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Impact

  • Loss of Funds: Users lose their previously locked tokens when creating a new lock.

Tools Used

  • Foundry: Used to write and execute the test suite that demonstrates the vulnerability.

  • Manual Review.

Recommendations

  1. Check for Existing Locks:

    • Before creating a new lock, check if the user already has an existing lock and revert if they do. This ensures that users cannot overwrite existing locks.

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();
​
// Check for existing lock
+ LockPosition memory existingLock = getLockPosition(msg.sender);
+ if (existingLock.amount > 0) revert ExistingLockFound();
​
// Other logic ...
​

2.Prevent Multiple Locks and recommend users to use the veRAACToken::increase function:

function increase(uint256 amount) external nonReentrant whenNotPaused {}
​
Updates

Lead Judging Commences

inallhonesty Lead Judge 7 months 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.

Give us feedback!