Summary
`veRaacToken::lock` function transfers Raac tokens from ms.sender and creates the lock without checking if msg.sender has an existing lock.
Users may loose tokens transferred in previous lock calls.
Vulnerability Details
Users can call lock function to create a new lock position and mint veRaac tokens.
First, the amount and duration are checked to not surpass the maximum values configured.
Then the amount of Raac token is transferred from msg.sender, the lockState.createLock is called to create the actual lock position and, in the end, the newPower amount of veRaac is minted.
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);
}
function createLock(
LockState storage state,
address user,
uint256 amount,
uint256 duration
) internal returns (uint256 end) {
if (state.minLockDuration != 0 && state.maxLockDuration != 0) {
if (duration < state.minLockDuration || duration > state.maxLockDuration)
revert InvalidLockDuration();
}
if (amount == 0) revert InvalidLockAmount();
end = block.timestamp + duration;
state.locks[user] = Lock({
amount: amount,
end: end,
exists: true
});
state.totalLocked += amount;
emit LockCreated(user, amount, end);
return end;
}
The lock function doesn't check if the caller has an existing lock position.
An user can call lock twice with the intention to create two different positions with different arguments - amount and lock duration.
However, the first lock position will be overwritten by the last position created.As a result, after the period expires, the user will be able to withdraw only the last locked amount.
Add the following test under Lock Mechanism test suite from
test/unit/core/tokens/veRAACToken.test.js file.
Run the test with npx hardhat test --grep "should allow users to increase lock amount" command.
it("should increase the user's locked amount when locking more than once", async () => {
const amount = ethers.parseEther("1000");
const halfAmount = ethers.parseEther("500");
const totalAmount = ethers.parseEther("1500");
const duration = 365 * 24 * 3600;
await veRAACToken.connect(users[0]).lock(amount, duration);
const firstLockedPosition = await veRAACToken.getLockPosition(users[0].address);
expect(firstLockedPosition.amount).to.equal(amount);
await veRAACToken.connect(users[0]).lock(halfAmount, duration);
const secondLockedPosition = await veRAACToken.getLockPosition(users[0].address);
expect(secondLockedPosition.amount).to.equal(totalAmount);
});
The test will fail with the following output:
Lock Mechanism
should allow users to increase lock amount:
AssertionError: expected 500000000000000000000 to equal 1500000000000000000000.
+ expected - actual
-500000000000000000000
+1500000000000000000000
Impact
Users may loose locked Raac tokens.
Tools Used
Recommendations
Add the following check in lock function:
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();
+ if(_lockState.locks[msg.sender].exists) revert OnlyOneLockPerAddressAllowed();
...
}
