Summary
The veRAACToken::lock() function unintentionally overwrites the existing lock data in _lockState.locks[user], potentially causing users to lose all previously locked tokens.
Vulnerability Details
The lock() function is designed to allow users to lock tokens. However, as indicated by the @> mark, the createLock() function in _lockState directly updates _lockState.locks[user]. Since the function does not check for an existing lock before writing new data, calling lock() again will overwrite the user's previous lock, effectively erasing all previously locked tokens.
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();
raacToken.safeTransferFrom(msg.sender, address(this), amount);
uint256 unlockTime = block.timestamp + duration;
@> _lockState.createLock(msg.sender, amount, duration);
_updateBoostState(msg.sender, amount);
(int128 bias, int128 slope) = _votingState.calculateAndUpdatePower(
msg.sender,
amount,
unlockTime
);
uint256 newPower = uint256(uint128(bias));
_checkpointState.writeCheckpoint(msg.sender, newPower);
_mint(msg.sender, newPower);
emit LockCreated(msg.sender, amount, unlockTime);
}
Poc
The following test demonstrates the issue by creating a lock and then calling lock() again with a different amount, which overwrites the original lock data.
Add the following test to test/unit/core/tokens/veRAACToken.test.js and execute it:
describe("Lock accidental overwriting of data", () => {
it("Poc", async () => {
const userBalanceStart = await raacToken.balanceOf(users[0].address);
const amount = ethers.parseEther("1000");
const duration = 365 * 24 * 3600 * 4 ;
const tx = await veRAACToken.connect(users[0]).lock(amount, duration);
const receipt = await tx.wait();
const event = receipt.logs.find(
log => log.fragment && log.fragment.name === 'LockCreated'
);
const actualUnlockTime = event.args[2];
const position = await veRAACToken.getLockPosition(users[0].address);
expect(position.amount).to.equal(amount);
expect(position.end).to.equal(actualUnlockTime);
expect(position.power).to.be.equal(amount);
await veRAACToken.connect(users[0]).lock(ethers.parseEther("1"), duration);
const checkPosition = await veRAACToken.getLockPosition(users[0].address);
expect(checkPosition.amount).to.equal(ethers.parseEther("1"));
await time.increase(365 * 24 * 3600 * 4 + 100);
await network.provider.send("evm_mine");
await veRAACToken.connect(users[0]).withdraw();
const userBalanceEnd = await raacToken.balanceOf(users[0].address);
console.log("users[0] loss amount:",userBalanceStart - userBalanceEnd);
});
});
output:
veRAACToken
Lock accidental overwriting of data
users[0] loss amount: 1000000000000000000000n
Impact
A user who calls lock() more than once will unintentionally overwrite their existing lock, causing them to lose all previously locked tokens.
Tools Used
Manual Review
Recommendations
To prevent accidental data overwriting, add a check to _lockState.locks[msg.sender].amount and revert if a lock already exists:, for example:
+ error lockDataAlreadyExists();
function lock(uint256 amount, uint256 duration) external nonReentrant whenNotPaused {
if (amount == 0) revert InvalidAmount();
if (amount > MAX_LOCK_AMOUNT) revert AmountExceedsLimit();
+ if (_lockState.locks[msg.sender].amount > 0) revert lockDataAlreadyExists();
// SNIP...
}
