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

If the value of `maxLockingDuration` is decreased while there are locked deposits in process, new deposits to those `lockIds` will be reverted

0xbepresent

Summary

If the value of maxLockingDuration is decreased while there are locked deposits in process, the deposit to those lockIds will be reverted, not allowing users to deposit more to their lockId.

Vulnerability Details

The user can add amounts multiple times to the same lockId with the help of the function SDLPoolPrimary::_storeUpdatedLock, this will cause it to be deposited to the same lockId if the user so wishes. The problem arises when the variable maxLockingDuration is decremented using the function LinearBoostController::setMaxLockingDuration, causing subsequent user deposits to the same lockId be reversed.

The following test shows how the user can deposit multiple times to the same lockId=1, however once maxLockingDuration is decreased, subsequent deposits for the same lockId will be reversed, making the user no longer able to deposit to the same lockId. Test steps:

  1. User stakes 100 tokens using the max locking duration (4 years).

  2. User deposits another 100 tokens to the lockId=1 using the same duration 4 years.

  3. Now the admin reduces the max locking duration to 2 years.

  4. User deposits more tokens to the lockId=1 using the same duration 4 years (like the step 2). The transaction will be reverted by MaxLockingDurationExceeded()

  5. The user tries to reduce the duration time to an accepted time (1 year) and deposits to the same lockId=1 but the transaction will be reverted by InvalidLockingDuration

// Filename: test/core/sdlPool/sdl-pool-primary.test.ts
// $ yarn test --grep "codehawks: boost change maxLockDuration"
//
it('codehawks: boost change maxLockDuration', async () => {
//
// 1. User stakes 100 tokens using the max locking duration (4 years)
await sdlToken.transferAndCall(
sdlPool.address,
toEther(100),
ethers.utils.defaultAbiCoder.encode(['uint256', 'uint64'], [0, 4 * 365 * DAY])
)
let ts1 = (await ethers.provider.getBlock(await ethers.provider.getBlockNumber())).timestamp
assert.equal(fromEther(await sdlToken.balanceOf(sdlPool.address)), 100)
// User owns the lockId=1
assert.deepEqual(parseLocks(await sdlPool.getLocks([1])), [
{ amount: 100, boostAmount: 400, startTime: ts1, duration: 4 * 365 * DAY, expiry: 0 },
])
assert.equal(await sdlPool.ownerOf(1), accounts[0])
assert.equal(fromEther(await sdlPool.effectiveBalanceOf(accounts[0])), 500) // 100 tokens + 400 boostAmount
//
// 2. User deposits another 100 tokens to the lockId=1 using the same duration 4 years, everything is ok
// now the effectiveBalance=1000
await sdlToken.transferAndCall(
sdlPool.address,
toEther(100),
ethers.utils.defaultAbiCoder.encode(['uint256', 'uint64'], [1, 4 * 365 * DAY])
)
assert.equal(fromEther(await sdlPool.effectiveBalanceOf(accounts[0])), 1000) // 200 tokens + 800 boostAmount
//
// 3. Now the admin reduces the locking duration to 2 years
await boostController.setMaxLockingDuration(2 * 365 * DAY)
//
// 4. User deposits more tokens to the lockId=1 using the same duration (4 years). The transaction
// will be reverted by MaxLockingDurationExceeded()
await expect(sdlToken.transferAndCall(
sdlPool.address,
toEther(20),
ethers.utils.defaultAbiCoder.encode(['uint256', 'uint64'], [1, 4 * 365 * DAY])
)).to.be.revertedWith('MaxLockingDurationExceeded()')
//
// 5. The user tries to reduce the duration time to an accepted time (1 year) and deposits to the same lockId=1
// but the transaction will be reverted by InvalidLockingDuration
await expect(sdlToken.transferAndCall(
sdlPool.address,
toEther(20),
ethers.utils.defaultAbiCoder.encode(['uint256', 'uint64'], [1, 365 * DAY])
)).to.be.revertedWith('InvalidLockingDuration');
})

Impact

The user will not be able to deposit tokens to the same lockId if the maxLockingDuration decreases, even if the user reduces its duration, the transaction will be reversed.

Tools used

Manual review

Recommendations

Allow deposits to the same lockId even if maxLockingDuration decreases. Modifying maxLockingDuration should not affect user deposits. Another solution could be to not allow the decrease of maxLockingDuration, only allow the increase.

Updates

Lead Judging Commences

0kage Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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