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

The `_transfer` function in the `SDLPool.sol` contract executes logic before updating rewards and transferring lock ownership

0xVinylDavyl

Summary

Vulnerability: Reentrancy Vulnerability in _transfer Function

Description: The _transfer function in the SDLPool.sol contract executes logic before updating rewards and transferring lock ownership. This leaves the contract vulnerable to reentrancy attacks, where an external contract could maliciously call back into the SDLPool contract before the state is updated.

Vulnerability Details

Code from SDLPool.sol

// In SDLPool.sol
function _transfer(
address _from,
address _to,
uint256 _lockId
) internal virtual {
if (_from != ownerOf(_lockId)) revert TransferFromIncorrectOwner();
if (_to == address(0)) revert TransferToZeroAddress();
if (_to == ccipController) revert TransferToCCIPController();
​
// Vulnerable to reentrancy attack here
delete tokenApprovals[_lockId];
​
_updateRewards(_from);
_updateRewards(_to);
​
uint256 effectiveBalanceChange = locks[_lockId].amount + locks[_lockId].boostAmount;
effectiveBalances[_from] -= effectiveBalanceChange;
effectiveBalances[_to] += effectiveBalanceChange;
​
balances[_from] -= 1;
balances[_to] += 1;
lockOwners[_lockId] = _to;
​
emit Transfer(_from, _to, _lockId);
}

Impact

  1. Reentrancy Risk:
    The vulnerable _transfer function allows an external contract to potentially reenter the SDLPool contract before the state is fully updated. This could lead to unexpected behavior and may be exploited by attackers to manipulate the contract's state during the execution of the function.

Tools Used

Manual Code Review

Recommendations

To mitigate the reentrancy vulnerability, it is recommended to follow the check-effects-interactions pattern. This involves updating the contract's state after performing external calls. In this case, move the _updateRewards calls to the end of the function, after all internal state changes are made. This helps ensure that external calls are the last actions in the function, reducing the risk of reentrancy attacks.

Mitigation Steps

// In SDLPool.sol
function _transfer(
address _from,
address _to,
uint256 _lockId
) internal virtual {
if (_from != ownerOf(_lockId)) revert TransferFromIncorrectOwner();
if (_to == address(0)) revert TransferToZeroAddress();
if (_to == ccipController) revert TransferToCCIPController();
​
// Move state-changing logic to the end
uint256 effectiveBalanceChange = locks[_lockId].amount + locks[_lockId].boostAmount;
​
delete tokenApprovals[_lockId];
​
balances[_from] -= 1;
balances[_to] += 1;
lockOwners[_lockId] = _to;
effectiveBalances[_from] -= effectiveBalanceChange;
effectiveBalances[_to] += effectiveBalanceChange;
​
emit Transfer(_from, _to, _lockId);
​
// Update rewards after state changes
_updateRewards(_from);
_updateRewards(_to);
}
Updates

Lead Judging Commences

0kage Lead Judge almost 2 years ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Support

FAQs

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