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

Using veRAACToken `lock(..)` instead of `increase(..)` locks funds in escrow

hardlydifficult

Summary

veRAACToken allows people to enter using the lock(..) function. Once entered, it's expected that users will increase(..) or extend(..) their position, however if they instead call lock(..) again then their original balance is locked in escrow.

Finding Description

veRAACToken.sol (in scope) uses LockManager.sol (out of scope) to help track state which has methods to createLock, increaseLock or extendLock. Looking at the create flow we see that it will write to storage without first considering existing values (which is reasonable to do on "create"):

function createLock(
LockState storage state,
address user,
uint256 amount,
uint256 duration
) internal returns (uint256 end) {
...
state.locks[user] = Lock({
amount: amount,
end: end,
exists: true
});
...
}

Source: LockManager.sol#L133-L137

In veRAACToken, we see that the lock function will use that createLock function without first checking to see if one has already been created:

function lock(uint256 amount, uint256 duration) external nonReentrant whenNotPaused {
...
_lockState.createLock(msg.sender, amount, duration);
...
}

Source: veRAACToken.sol#L226

Once a lock has been created, veRAACToken.sol includes increase and extend functions which are meant to be used instead of calling lock again:

function increase(uint256 amount) external nonReentrant whenNotPaused {
_lockState.increaseLock(msg.sender, amount);
...
}
​
function extend(uint256 newDuration) external nonReentrant whenNotPaused {
uint256 newUnlockTime = _lockState.extendLock(msg.sender, newDuration);
...
}

Source: veRAACToken.sol#L251-L305

However if lock is called multiple times, data is overwritten and the original deposit is lost. This surfaces as a problem when a user attempts to withdraw after the lock expired, which uses the amount stored to know how many tokens to return to the user:

function withdraw() external nonReentrant {
LockManager.Lock memory userLock = _lockState.locks[msg.sender];
...
uint256 amount = userLock.amount;
...
delete _lockState.locks[msg.sender];
...
raacToken.safeTransfer(msg.sender, amount);
...
}

Source: veRAACToken.sol#L329

As a result, the user can only withdraw their most recent deposit. Funds from their original lock call are locked in the contract's escrow. Even emergencyWithdraw will fail to access these funds for the same reason:

function emergencyWithdraw() external nonReentrant {
...
LockManager.Lock memory userLock = _lockState.locks[msg.sender];
...
uint256 amount = userLock.amount;
...
delete _lockState.locks[msg.sender];
...
raacToken.safeTransfer(msg.sender, amount);
...
}

Source: veRAACToken.sol#L367-L383

Impact Explanation

The user loses their RAAC tokens and those tokens are effectively burned due to being locked in escrow. The escrow contract is not upgradeable and emergency controls cannot recover the funds either.

Likelihood Explanation

Even if the frontend UI attempts to avoid this issue by always using the increase or extend functions, it's possible for it to incorrectly suggest using lock again. For example, maybe the first lock is still a transaction in the mempool or maybe the app's backend is behind on processing events. It seems likely that some people will be impacted by this bug.

Proof of Concept

This git patch shows adding a test to the e2e suite, taking advantage of the before steps there. Run with npm run test:e2e.

Here we have a user lock tokens twice and then show that they are unable to withdraw tokens from their first lockup:

diff --git a/test/e2e/protocols-tests.js b/test/e2e/protocols-tests.js
index 3040a7e..ffcb7ce 100644
--- a/test/e2e/protocols-tests.js
+++ b/test/e2e/protocols-tests.js
@@ -32,6 +32,50 @@ describe('Protocol E2E Tests', function () {
}
});
+ it.only('BUG', async function () {
+ const LARGE_STAKE_AMOUNT = ethers.parseEther('500');
+ const SMALL_REUP_STAKE_AMOUNT = ethers.parseEther('20');
+
+ // Approve the token spend:
+ await contracts.raacToken.connect(user2).approve(contracts.veRAACToken.target, LARGE_STAKE_AMOUNT * 2n);
+
+ // First we lock with a large amount:
+ await contracts.veRAACToken.connect(user2).lock(LARGE_STAKE_AMOUNT, ONE_YEAR);
+ // Escrow 500 WAD of RAAC and get 125 WAD of veRAACTokens.
+
+ // Then, sometime later we lock again but with a much smaller amount:
+ await contracts.veRAACToken.connect(user2).lock(SMALL_REUP_STAKE_AMOUNT, ONE_YEAR);
+ // Escrow 20 WAD of RAAC and get 5 WAD of veRAACTokens.
+
+ // Sanity check:
+ {
+ // The user has 480 WAD of RAAC left (spent 520 WAD):
+ expect(await contracts.raacToken.balanceOf(user2)).to.eq(INITIAL_MINT_AMOUNT - LARGE_STAKE_AMOUNT - SMALL_REUP_STAKE_AMOUNT);
+ // The user got 130 WAD of veRAAC minted in total:
+ expect(await contracts.veRAACToken.balanceOf(user2)).to.eq(130n * 10n ** 18n);
+ }
+
+ // After the lock has expired...
+ await time.increase(ONE_YEAR);
+
+ // The user withdraws their position:
+ const tx = await contracts.veRAACToken.connect(user2).withdraw();
+
+ // The user no longer has any veRAACTokens:
+ await expect(tx).to.changeTokenBalance(contracts.veRAACToken, user2, -130n * 10n ** 18n);
+ expect(await contracts.veRAACToken.balanceOf(user2)).to.eq(0);
+
+ // BUG: However they only got back the smaller amount:
+ await expect(tx).to.changeTokenBalance(contracts.raacToken, user2, SMALL_REUP_STAKE_AMOUNT);
+ expect(await contracts.raacToken.balanceOf(user2)).to.eq(INITIAL_MINT_AMOUNT - LARGE_STAKE_AMOUNT);
+
+ // BUG: The original large lock is still in escrow and cannot be withdrawn:
+ expect(await contracts.raacToken.balanceOf(contracts.veRAACToken.target)).to.eq(LARGE_STAKE_AMOUNT);
+
+ // At this point even an `emergencyWithdraw` would fail since the user's lock state has been deleted.
+ });
+
+
describe('RAAC Token', function () {
const TRANSFER_AMOUNT = ethers.parseEther('100');

Recommendation

The following patch adds a check so that calling lock reverts when one already exists. This way users are forced to use increase or extend as expected, until they are able to withdraw. After withdraw is called, they are free to start over and call lock once more.

When applied, the POC test above fails (since the second call to lock now reverts).

diff --git a/contracts/core/tokens/veRAACToken.sol b/contracts/core/tokens/veRAACToken.sol
index 71c040c..2ead1eb 100644
--- a/contracts/core/tokens/veRAACToken.sol
+++ b/contracts/core/tokens/veRAACToken.sol
@@ -166,6 +166,8 @@ contract veRAACToken is ERC20, Ownable, ReentrancyGuard, IveRAACToken {
delete _emergencyTimelock[actionId];
}
+ error LockAlreadyExists();
+
/**
* @notice Initializes the veRAACToken contract
* @param _raacToken Address of the RAAC token contract
@@ -215,6 +217,7 @@ contract veRAACToken is ERC20, Ownable, ReentrancyGuard, IveRAACToken {
if (totalSupply() + amount > MAX_TOTAL_SUPPLY) revert TotalSupplyLimitExceeded();
if (duration < MIN_LOCK_DURATION || duration > MAX_LOCK_DURATION)
revert InvalidLockDuration();
+ if (_lockState.getLock(msg.sender).exists) revert LockAlreadyExists();
// Do the transfer first - this will revert with ERC20InsufficientBalance if user doesn't have enough tokens
raacToken.safeTransferFrom(msg.sender, address(this), amount);
Updates

Lead Judging Commences

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