An attacker can steal assets due to an incorrect revertInvalidWithdrawalConditions check in DelegateToken.withdraw()

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-09-delegate/blob/main/src/libraries/DelegateTokenStorageHelpers.sol#L162-L167

Vulnerability details

Impact

Function withdraw() (https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L353-L386) - allows principal rights owner or approved operator to withdraw the underlying token once the delegation rights have either met their expiration or been rescinded. To withdraw assets, a check is made that the assets are being withdrawn either by the owner, an authorized operator, or an approved user. However, the check is not performed correctly. This allows you to steal any assets.

Proof of Concept

The line https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L359 calls the function revertInvalidWithdrawalConditions() (https://github.com/code-423n4/2023 -09-delegate/blob/main/src/libraries/DelegateTokenStorageHelpers.sol#L155-L168):

function revertInvalidWithdrawalConditions( mapping(uint256 delegateTokenId => uint256[3] info) storage delegateTokenInfo, mapping(address account => mapping(address operator => bool enabled)) storage accountOperator, uint256 delegateTokenId, address delegateTokenHolder ) internal view { if (block.timestamp < readExpiry(delegateTokenInfo, delegateTokenId)) { if (delegateTokenHolder == msg.sender || msg.sender == readApproved(delegateTokenInfo, delegateTokenId) || accountOperator[delegateTokenHolder][msg.sender]) { return; } revert Errors.WithdrawNotAvailable(delegateTokenId, readExpiry(delegateTokenInfo, delegateTokenId), block.timestamp); } }

The function functions without raising an error when the condition is met:

if (delegateTokenHolder == msg.sender || msg.sender == readApproved(delegateTokenInfo, delegateTokenId) || accountOperator[delegateTokenHolder][msg.sender]) { return; }

However, this check is performed only when the following condition is met:

if (block.timestamp < readExpiry(delegateTokenInfo, delegateTokenId))

Thus, if an attacker calls withdraw() with a delegateTokenId whose expiry exceeds block.timestamp, he can obtain the delegateTokenId assets for himself (https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken .sol#L369, https://github.com/code-423n4/2023-09-delegate/blob/main/src/DelegateToken.sol#L375, https://github.com/code-423n4/2023-09 -delegate/blob/main/src/DelegateToken.sol#L384)

Tools Used

Manual review

Recommended Mitigation Steps

Place revert Errors.WithdrawNotAvailable(delegateTokenId, readExpiry(delegateTokenInfo, delegateTokenId), block.timestamp); outside if(block.timestamp < readExpiry(delegateTokenInfo, delegateTokenId)) {...}

Assessed type

Context

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid

[c4-judge]

GalloDaSballo removed the grade

[c4-judge]

GalloDaSballo marked the issue as duplicate of #282

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Invalid