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

Creating a lock twice leads to loss of funds and improper accounting

tejaswarambhe

Summary

The veRAACToken::lock function does not checks if the user already holds a lock or not, hence if a user creates a lock twice, the old position is lost along with the funds.

Vulnerability Details

The veRAACToken::lock function is designed to allow users to lock their RAACTokens in order to obtain voting rights which is represented as veRAACTokens.

function lock(uint256 amount, uint256 duration) external nonReentrant whenNotPaused {
// . . . Rest of the code . . .
​
// Do the transfer first - this will revert with ERC20InsufficientBalance if user doesn't have enough tokens
raacToken.safeTransferFrom(msg.sender, address(this), amount); <<@ -- // Raac Tokens are taken here
// Calculate unlock time
uint256 unlockTime = block.timestamp + duration;
// Create lock position
_lockState.createLock(msg.sender, amount, duration);
_updateBoostState(msg.sender, amount);
​
// . . . Rest of the code . . .
​
// Mint veTokens
_mint(msg.sender, newPower); <<@ -- // veRAAC Tokens are minted here.
​
emit LockCreated(msg.sender, amount, unlockTime);
}

However, upon observation we can determine that this function does not check if the user holds any current position in the _lockState.
Hence, if a user decides to create a new lock with either intention of creating a different lock period or simply increasing their voting power or simply a mistake, the old position would be over-written leading to loss of funds as the withdraw veRAACToken::withdraw function uses the lock state's locked amount.

function withdraw() external nonReentrant {
LockManager.Lock memory userLock = _lockState.locks[msg.sender];
if (userLock.amount == 0) revert LockNotFound();
if (block.timestamp < userLock.end) revert LockNotExpired();
​
uint256 amount = userLock.amount; <<@ - // Uses userLock.amount instead of msg.sender

Impact

  1. Overwrittes state with new duration.

  2. Loss of funds as the withdraw function leverages the over-written state for withdrawal.

  3. The view function veRAACToken::_updateBoostState is incorrect as the boostState.totalWeight will now contain incorrect value which is updated when tokens are locked, but as we withdraw the old veRAAC tokens are not actually locked here but burnt and the accounting can be deemed lost in oblivion.

Proof of Concept

Add the following test case inside the veRAACToken.test.js file:

describe("Improper Lock Mechanism", () => {
it("should allow creating a lock twice", async () => {
​
const amount = ethers.parseEther("100");
const duration = 365 * 24 * 3600; // 1 year
// Create lock first with 100 RAAC tokens
const tx = await veRAACToken.connect(users[0]).lock(amount, duration);
// Wait for the transaction
const receipt = await tx.wait();
// Find the LockCreated event
const event = receipt.logs.find(
log => log.fragment && log.fragment.name === 'LockCreated'
);
​
const position = await veRAACToken.getLockPosition(users[0].address);
console.log("Position 1: ", position);
// Position 1: Result(3) [ 100000000000000000000n, 1771844618n, 25000000000000000000n]
​
// Overwritting with a smaller amount
const tx1 = await veRAACToken.connect(users[0]).lock(ethers.parseEther('10'), duration);
const receipt1 = await tx1.wait();
const event1 = receipt1.logs.find(
log => log.fragment && log.fragment.name === 'LockCreated'
);
const position1 = await veRAACToken.getLockPosition(users[0].address);
​
// Overwritten position
console.log("Position 2: ", position1);
// Position 2: Result(3) [ 10000000000000000000n, 1771844621n, 27500000000000000000n ]
});
});

Tools Used

Manual Review

Hardhat

Recommendations

It is recommended to check if there's a lock present:

function lock(uint256 amount, uint256 duration) external nonReentrant whenNotPaused {
LockManager.Lock memory userLock = _lockState.locks[msg.sender];
if (userLock.amount > 0) revert LockFound();
// . . . Rest of the code . . .
}
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!