stake.link
stake.link
DeFiHardhatBridge
27,500 USDC
View results
Previous Next
Submission Details
Severity: medium
Valid

The user will lose their tokens on the `secondary chain` if they decide to withdraw all their tokens, then for some reason back out and deposit tokens for the same `lockId`

0xbepresent

Summary

The user will lose their tokens on the secondary chain if they decide to withdraw all their tokens, then for some reason back out and deposit tokens for the same lockId.

Vulnerability Details

The user can send sdl tokens on the secondary chain by queuing the action in SDLPoolSecondary::_queueNewLock, after the system is synchronized with primary chain that token is created with the help of the function SDLPoolSecondary::_mintQueuedNewLocks. Likewise, the user can withdraw their tokens with the help of the function SDLPoolSecondary::withdraw which will enqueue the action and once the system is synchronized with primary chain, the user will get his tokens.

The problem arises when the user deposits tokens again once he has decided to withdraw all their tokens, this will cause them to lose their last deposited tokens. Please see the following test:

  1. User deposits 100 tokens on secondary chain

  2. The system updates the new supply to the primary chain using the handleOutgoingUpdate(). Now user mints the new token lockId=1.

  3. User withdraws all his 100 tokens, the action is queued and the new supply is send to primary chain in order to be synchronized.

  4. User for some reason retracts and deposits 20 tokens to the lockId=1, this action is executed before the Controller returns the response to the secondary chain (step 3). Now the user has two queued actions, the action which the amount is 0 (withdrawal) and the action which the user deposit 20 tokens

  5. Finally the controller updates the new info and the user execute the queued operations for the current batch using the SDLPoolSecondary::_executeQueuedLockUpdates. At this point the withdrawal is executed and the lockId is removed. The user still has the 20 tokens deposit action queued in the getQueuedLockUpdates() function.

  6. System sends the new batch to L1 and then the controller returns the response.

  7. Now the user wants to execute the queued 20 tokens deposit using the SDLPoolSecondary::_executeQueuedLockUpdates and it will be reverted because the lockId was removed in the step 5. User wants to withdraw and also it will be reverted. User lost his tokens.

// Filename: test/core/sdlPool/sdl-pool-secondary.test.ts
// $ yarn test --grep "codehawks: mint, withdraw, deposit then revert"
//
it('codehawks: mint, withdraw, deposit then revert', async () => {
//
// 1. User deposits 100 tokens
await sdlToken.transferAndCall(
sdlPool.address,
toEther(100),
ethers.utils.defaultAbiCoder.encode(['uint256', 'uint64'], [0, 0])
)
assert.deepEqual(parseNewLocks(await sdlPool.getQueuedNewLocksByOwner(accounts[0])), [
[{amount: 100, boostAmount: 0, startTime: 0, duration: 0, expiry: 0,},],
[1],
])
//
// 2. User mints the new token (lockId=1)
await sdlPool.setCCIPController(accounts[0])
await sdlPool.handleOutgoingUpdate()
await sdlPool.handleIncomingUpdate(1)
await sdlPool.executeQueuedOperations([])
assert.equal(fromEther(await sdlPool.totalEffectiveBalance()), 100)
assert.equal(await sdlPool.ownerOf(1), accounts[0])
// the queue new lock is empty now
assert.deepEqual(parseNewLocks(await sdlPool.getQueuedNewLocksByOwner(accounts[0])), [[], [],])
//
// 3. User withdraws all his tokens (100), the action is queued and the new supply is send to primary chain
await sdlPool.withdraw(1, toEther(100))
await sdlPool.handleOutgoingUpdate() // data is sent to L1
assert.deepEqual(parseLockUpdates(await sdlPool.getQueuedLockUpdates([1])), [
[
{
updateBatchIndex: 2,
lock: { amount: 0, boostAmount: 0, startTime: 0, duration: 0, expiry: 0 },
},
],
])
//
// 4. User deposits some tokens to the lockId=1 before the Controller returns the response
await sdlToken.transferAndCall(
sdlPool.address,
toEther(20),
ethers.utils.defaultAbiCoder.encode(['uint256', 'uint64'], [1, 0])
)
// User has a list of two arrays
assert.deepEqual(parseLockUpdates(await sdlPool.getQueuedLockUpdates([1])), [
[
{
updateBatchIndex: 2,
lock: { amount: 0, boostAmount: 0, startTime: 0, duration: 0, expiry: 0 },
},
{
updateBatchIndex: 3,
lock: { amount: 20, boostAmount: 0, startTime: 0, duration: 0, expiry: 0 },
},
],
])
//
// 5. Finally the controller updates the new info and the queued operations for the current batch
// and lockId=1 are executed
await sdlPool.handleIncomingUpdate(2)
await sdlPool.executeQueuedOperations([1])
assert.equal(fromEther(await sdlPool.totalEffectiveBalance()), 0)
assert.equal(fromEther(await sdlToken.balanceOf(sdlPool.address)), 20);
// The user still has the deposit of the 20 tokens in queue
assert.deepEqual(parseLockUpdates(await sdlPool.getQueuedLockUpdates([1])), [
[
{
updateBatchIndex: 3,
lock: { amount: 20, boostAmount: 0, startTime: 0, duration: 0, expiry: 0 },
},
],
])
//
// 6. System sends the new batch to L1 and the L1 returns the response
await sdlPool.handleOutgoingUpdate()
await sdlPool.handleIncomingUpdate(3)
//
// 7. User wants to execute the queued deposit and it will be reverted.
// User wants to withdraw and also it will be reverted.
// User lost his tokens.
await expect(sdlPool.executeQueuedOperations([1])).to.be.revertedWith('InvalidLockId()')
await expect(sdlPool.withdraw(1, toEther(20))).to.be.revertedWith('InvalidLockId()')
assert.equal(fromEther(await sdlPool.totalEffectiveBalance()), 0)
assert.equal(fromEther(await sdlToken.balanceOf(sdlPool.address)), 20); // pool still has the user's 20 tokens deposit
// The user still has his queue deposit tokens
assert.deepEqual(parseLockUpdates(await sdlPool.getQueuedLockUpdates([1])), [
[
{
updateBatchIndex: 3,
lock: { amount: 20, boostAmount: 0, startTime: 0, duration: 0, expiry: 0 },
},
],
])
})

Impact

The user loses tokens.

Tools used

Manual review

Recommendations

If the user withdraws all his tokens from a lockId, no longer allow a subsequent deposit to the same lockId since those tokens will no longer be recovered.

Updates

Lead Judging Commences

0kage Lead Judge
almost 2 years ago
0kage Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

add-to-old-lock

User trying to update a fully withdrawn lock in same batch id on secondary pool

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.