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

Logical Error/Improper Lock Duration Update, in SDLPool::_updateLock, resulting in Potential Violation of Locking Constraints

azmaeengh

Summary

The _updateLock function in the SDLPool contract is intended to update the terms of an existing lock, specifically the amount and the locking duration. However, there is a logic flaw that allows the locking duration to be decreased even after the lock has expired, which should not be possible according to the intended behavior of the contract.

Vulnerability Details

Issue:

function _updateLock(
Lock memory _lock,
uint256 _amount,
uint64 _lockingDuration
) internal view returns (Lock memory) {
@> if ((_lock.expiry == 0 || _lock.expiry > block.timestamp) && _lockingDuration < _lock.duration) {
revert InvalidLockingDuration();
}
// ... (rest of the function)
}

The issue arises from the conditional check that only reverts if the lock has not expired and the new locking duration is less than the current duration. If the lock has already expired (_lock.expiry != 0 && _lock.expiry <= block.timestamp), the first condition will be false, and the whole expression within the if statement will be false, so the function will not revert, even if the new locking duration is less than the current duration. This is the potential issue, as it allows the locking duration to be decreased for expired locks, which may not be the intended behavior.

POC :

Note: Written in Foundry
Note: The test was done using only 1 import of SDLPool.sol file (since I have others files associated with it) from the contracts.

TEST CODE (in foundry)
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.15;
​
import "forge-std/Test.sol";
import "../src/SDLPool.sol"; // Adjust the import path as necessary
​
// (Assuming that all the imports are in relevant places).
​
// TestableSDLPool inherits from SDLPool and exposes functions for testing
contract TestableSDLPool is SDLPool {
constructor(
string memory _name,
string memory _symbol,
address _sdlToken,
address _boostController
) {
// Directly set the state variables instead of using an initializer
name = _name;
symbol = _symbol;
sdlToken = IERC20Upgradeable(_sdlToken);
boostController = IBoostController(_boostController);
}
​
// Expose the internal _createLock function for testing
function createLock(uint256 _amount, uint64 _lockingDuration) public {
// Call the internal _createLock function
Lock memory newLock = _createLock(_amount, _lockingDuration);
// Additional logic to add the lock to the contract's state, if necessary
locks[lastLockId] = newLock;
lockOwners[lastLockId] = msg.sender;
lastLockId++;
}
​
// Expose the internal _updateLock function for testing
function updateLock(
uint256 _lockId,
uint256 _amount,
uint64 _lockingDuration
) public {
// Call the internal _updateLock function
Lock memory updatedLock = _updateLock(
locks[_lockId],
_amount,
_lockingDuration
);
// Update the lock in the contract's state
locks[_lockId] = updatedLock;
}
​
// Function to get the details of a lock by its ID for testing
function getLock(uint256 _lockId) public view returns (Lock memory) {
return locks[_lockId];
}
}
​
/////////////////////
// MAIN TEST CONTRACT:
/////////////////////
// SDLPoolTest is the actual test contract that uses TestableSDLPool
contract SDLPoolTest is Test {
TestableSDLPool pool;
address owner;
​
function setUp() public {
owner = address(this);
pool = new TestableSDLPool("SDL Token", "SDL", address(1), address(2)); // Mock addresses for sdlToken and boostController
}
​
function testCannotDecreaseLockDurationAfterExpiry() public {
// Set up a lock with a certain duration
uint256 amount = 1000;
uint64 duration = 365 days;
pool.createLock(amount, duration);
​
uint256 lockId = pool.lastLockId(); // Assuming this returns the correct last lock ID
​
// Simulate time passing until after the lock's expiry
vm.warp(block.timestamp + duration + 1);
​
// Attempt to update the lock with a shorter duration
uint64 newDuration = 180 days; // Shorter duration
​
/////////////////////
// Since the lock has expired, we expect that the update will not revert
// even though the new duration is shorter than the original duration
/////////////////////
pool.updateLock(lockId, amount, newDuration);
}
}

Impact

This flaw could allow users to circumvent the intended locking mechanism, potentially withdrawing their tokens earlier than allowed or affecting the reward calculations that depend on the lock duration.

Tools Used

Manual Review, Audit Wizard tool, AI.

Recommendations

To resolve this issue, the conditional check should be updated to ensure that the locking duration cannot be decreased under any circumstances:

function _updateLock(
Lock memory _lock,
uint256 _amount,
uint64 _lockingDuration
) internal view returns (Lock memory) {
+ require(_lockingDuration >= _lock.duration, "New locking duration must be greater than or equal to current duration");
// ... (rest of the function)
}

By using a require statement, the contract enforces that the new locking duration must always be greater than or equal to the current duration, regardless of the lock's expiry status. This change ensures that the integrity of the locking mechanism is maintained and that the contract behaves as intended.

Updates

Lead Judging Commences

0kage Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Design choice

Support

FAQs

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