Summary
Let suppose , I locked come amount and give permission to someone as approval with SDLPool.approva then I go and call SDLPoolSecondary.withdraw all of the amount in it , still approval exists.
Vulnerability Details
Approval should be deleted or removed as you did in SDLPoolPrimary.withdraw, in SDLPoolPrimary , it check it withdraw amount is equal to baseAmount , simple delete the Locks and tokenApprovals[_lockId] on the Lock to create any misleading
SDLPoolPrimary.withdraw
uint256 baseAmount = locks[_lockId].amount;
if (_amount > baseAmount) revert InsufficientBalance();
emit Withdraw(msg.sender, _lockId, _amount);
if (_amount == baseAmount) {
delete locks[_lockId];
delete lockOwners[_lockId];
balances[msg.sender] -= 1;
if (tokenApprovals[_lockId] != address(0)) delete tokenApprovals[_lockId];
emit Transfer(msg.sender, address(0), _lockId);
} else {
locks[_lockId].amount = baseAmount - _amount;
}
effectiveBalances[msg.sender] -= _amount;
totalEffectiveBalance -= _amount;
SDLPoolSecondary.withdraw
if (_amount > baseAmount) revert InsufficientBalance();
lock.amount = baseAmount - _amount;
effectiveBalances[msg.sender] -= _amount;
totalEffectiveBalance -= _amount;
queuedLockUpdates[_lockId].push(LockUpdate(updateBatchIndex, lock));
queuedRESDLSupplyChange -= int256(_amount);
if (updateNeeded == 0) updateNeeded = 1;
emit QueueWithdraw(msg.sender, _lockId, _amount);
Test
it('should be able to withdraw and burn lock NFT', async () => {
let ts1 = await mintLock()
await time.increase(200 * DAY)
await sdlPool.initiateUnlock(1)
let ts2 = (await ethers.provider.getBlock(await ethers.provider.getBlockNumber())).timestamp
await time.increase(200 * DAY)
let startingBalance = await sdlToken.balanceOf(accounts[0])
await sdlPool.approve(accounts[1], 1)
let beforeWithdrawApprove = await sdlPool.getApproved(1);
console.log("beforeWithdrawApprove",beforeWithdrawApprove)
await sdlPool.withdraw(1, toEther(100))
let afterWithdrawApprove = await sdlPool.getApproved(1);
console.log("beforeWithdrawApprove",afterWithdrawApprove)
await updateLocks()
assert.equal(fromEther(await sdlPool.queuedRESDLSupplyChange()), 0)
assert.equal(fromEther((await sdlToken.balanceOf(accounts[0])).sub(startingBalance)), 100)
assert.equal(fromEther(await sdlPool.totalStaked()), 0)
}
Impact
When one who got the approval still think he can withdraw some amount , even their was nothing left.
Tools Used
Manual Review
Recommendations
Better is to delete the lock when someone take all of it amount from it and remove the Approval .
