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

A user can lose funds in `sdlPoolSecondary` if tries to add more sdl tokens to a lock that has been queued to be completely withdrawn

draiakoo

Summary

In a secondary chain, if a user adds more sdl amount into a lock that he has queued to withdraw all the amount in the same index batch, he will lose the extra amount he deposited

Vulnerability Details

The process to withdraw all the funds from a lock in a primary chain is just by calling withdraw with all the base amount of the lock. At this point the user will get immediately his funds back and the lock will be deleted, hence the owner will be zero address.

However, in a secondary chain, a user has to queue a withdraw of all the funds and wait for the keeper to send the update to the primary chain to execute the updates and then receive his sdl token back. In this period of time when the keeper does not send the update to the primary chain, if a user queues a withdraw of all the lock base amount, he will still own the lock because the withdraw has not been executed, just queued. So the user can still do whatever modification in his lock, for example, increase his lock base amount by calling transferAndCall() in the sdlToken passing the address of the sdlSecondaryPool as argument.

If this happens, when the keeper send the update to the primary chain and the user executes the updates for his lockId, he will lose this extra amount he deposited because it will execute the updates in order, and it will start with the withdraw of all the funds, will delete the ownership (make the zero address as the owner), and then increase the base amount of the lock that now owns the zero address.

And basically the lockId will be owned by the zero address with base amount as the extra sdl tokens that the user sent.

Proof of Concept

It is written inside sdl-pool-secondary.test.ts because it uses its setup

it('PoC user will lose extra deposited tokens', async () => {
​
let user = accounts[1]
let initialUserSDLBalance = await sdlToken.balanceOf(user);
​
// User creates a lock depositing some amount
await sdlToken
.connect(signers[1])
.transferAndCall(
sdlPool.address,
toEther(100),
ethers.utils.defaultAbiCoder.encode(['uint256', 'uint64'], [0, 0])
)
​
await sdlPool.handleOutgoingUpdate()
await sdlPool.handleIncomingUpdate(1)
await sdlPool.connect(signers[1]).executeQueuedOperations([])
​
assert.equal(await sdlPool.ownerOf(1), user)
// User queues a withdraw of all the amount from the lock
await sdlPool.connect(signers[1]).withdraw(1, toEther(100))
​
// User wants to deposit more tokens to the lock without the withdraw being updated and still being in the queue
await sdlToken
.connect(signers[1])
.transferAndCall(
sdlPool.address,
toEther(1000),
ethers.utils.defaultAbiCoder.encode(['uint256', 'uint64'], [1, 0])
)
​
await sdlPool.handleOutgoingUpdate()
await sdlPool.handleIncomingUpdate(2)
// When executing the updates, zero address will be the owner of his lock
// and the amount he diposited the last time will be lost
await sdlPool.connect(signers[1]).executeQueuedOperations([1])
​
let finalUserSDLBalance = await sdlToken.balanceOf(user);
let sdlLost = initialUserSDLBalance.sub(finalUserSDLBalance)
​
console.log("The user has lost", sdlLost.toString(), "sdl tokens")
​
// This staticall should revert because now the lock owner is the zero address
await expect(sdlPool.ownerOf(1)).to.be.revertedWith('InvalidLockId()')
})

Output:

SDLPoolSecondary
The user has lost 1000000000000000000000 sdl tokens
✔ PoC user is not able to execute his lock updates (159ms)
​
​
1 passing (3s)

Impact

High, user will lose funds

Tools Used

Manual review

Recommendations

When trying to do any action on a lock in a secondary pool, check if the last update queued has not 0 as the base amount. Because if it is the case, that would mean that the user queued a withdraw of all funds and he will lose ownership of the lock at the next keeper update.

function _queueLockUpdate(
address _owner,
uint256 _lockId,
uint256 _amount,
uint64 _lockingDuration
) internal onlyLockOwner(_lockId, _owner) {
Lock memory lock = _getQueuedLockState(_lockId);
+ if(lock.amount == 0) revert();
LockUpdate memory lockUpdate = LockUpdate(updateBatchIndex, _updateLock(lock, _amount, _lockingDuration));
queuedLockUpdates[_lockId].push(lockUpdate);
queuedRESDLSupplyChange +=
int256(lockUpdate.lock.amount + lockUpdate.lock.boostAmount) -
int256(lock.amount + lock.boostAmount);
if (updateNeeded == 0) updateNeeded = 1;
​
emit QueueUpdateLock(_owner, _lockId, lockUpdate.lock.amount, lockUpdate.lock.boostAmount, lockUpdate.lock.duration);
}
Updates

Lead Judging Commences

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.