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

`SDLPool.safeTransferFrom` is not reverting if `_lockId` is invalid, as expected and outlined in the comments

larsson

Summary

SDLPool.safeTransferFrom is not reverting if _lockId is invalid, as expected and outlined in the comments.

Vulnerability Details

The safeTransferFrom function is expected to check the validity of _lockId and revert in the opposite case. This is laid out clearly in the comments:

* @dev
* - calls onERC721Received on `_to` if it is a contract or reverts if it is a contract
* and does not implemement onERC721Received
* - reverts if sender is not the owner of and not approved to transfer the lock
@> * - reverts if `_lockId` is invalid

However, it does not apply since the function only checks for validity and approval of the msg.sender in the safeTransferFrom function

@> if (!_isApprovedOrOwner(msg.sender, _lockId)) revert SenderNotAuthorized();
_transfer(_from, _to, _lockId);
if (!_checkOnERC721Received(_from, _to, _lockId, _data)) revert TransferToNonERC721Implementer();

and reverts if _from is not the owner of the lock in the _transfer function

@> if (_from != ownerOf(_lockId)) revert TransferFromIncorrectOwner();
if (_to == address(0)) revert TransferToZeroAddress();
if (_to == ccipController) revert TransferToCCIPController();

Impact

This opens the door for an attacker to potentially exploit the issue by calling the function with an invalid or nonexistent _lockId parameter. The function will continue to execute without catching the error due to the lack of proper checks. This can cause the amount stored in mapping(uint256 => Lock) internal locks; and the uint256 effectiveBalanceChange to be updated before the error is caught later on.

Tools Used

VsCode

Recommendations

The function should implement an additional if() statement in safeTransferFrom.

function safeTransferFrom(
address _from,
address _to,
uint256 _lockId,
bytes memory _data
) public {
+ if (lockOwners[_lockId] == address(0)) revert InvalidLockId();
if (!_isApprovedOrOwner(msg.sender, _lockId)) revert SenderNotAuthorized();
_transfer(_from, _to, _lockId);
if (!_checkOnERC721Received(_from, _to, _lockId, _data)) revert TransferToNonERC721Implementer();
}
​
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.