search

sherlock-audit / 2023-11-convergence-judging

8 stars 8 forks source link

0x52 - LockPositionDelegate doesn't clear delegates on transfer which can be used to honeypot buyers #175

Closed sherlock-admin closed 9 months ago

sherlock-admin commented 10 months ago

0x52

high

LockPositionDelegate doesn't clear delegates on transfer which can be used to honeypot buyers

Summary

When transferring locked positions, the delegates are not cleared from the previous owner. This can be used to honeypot buyers. A malicious user would list their token which a large TDE claim available. They can then frontrun the legitimate user with a call setting themselves as the ysCVG delegate. Then after they can claim the TDE at the expense of the new owner who paid extra for the token since it was entitled to a TDE claim.

Vulnerability Detail

LockingPositionDelegate.sol#L237-L240

function delegateYsCvg(uint256 _tokenId, address _to) external onlyTokenOwner(_tokenId) {
    delegatedYsCvg[_tokenId] = _to;
    emit DelegateShare(_tokenId, _to);
}

delegateYsCvg allows the owner of the token to set the receiver of yield sharing. It is also the only function that sets the delegatedYsCvg mapping. LockingPositionManager uses the standard implementation of transfers which never clears out this delegation when the token is transferred. As a result of this, all delegation will persist across transfers. This enables the honeypot scenario described above.

Impact

Users can be honeypotted by malicious token sellers

Code Snippet

SdtStakingPositionManager.sol#L21

Tool used

Manual Review

Recommendation

Overwrite the afterTokenTransfer method to forcefully clear delegation when a token is transferred

shalbe-cvg commented 10 months ago

Hello,

Thanks a lot for your attention.

After an in-depth review, we have to consider your issue as Confirmed. You're right, after transferring (whether it's a purchase on a marketplace or a simple transfer) we should ensure that all delegatees associated to the token are removed. Otherwise, as you said, buyer (i.e. receiver) might not be aware of the delegatees and could be subject to a honeypot attack.

However we have decided to disagree with the severity and put it as a Medium issue.

Regards, Convergence Team

nevillehuang commented 9 months ago

Hi @0xR3vert @shalbe-cvg @walk-on-me,

I think this could remain as high severity given explicit TDE claim can be hijacked. Why do you think this constitutes medium severity?

nevillehuang commented 9 months ago

Keeping this as high severity given delegate logic allows malicious user to perform the honeypot without any external limitations.

CergyK commented 9 months ago

Escalate

This should be of low severity, since there is a locking mechanism to ensure that previous owner cannot take any malicious action on the NFT for some duration before and after a sale.

If the lock is set right during the sale, this provides perfect protection for the buyer:

Please check: https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Token/CvgERC721TimeLockingUpgradeable.sol#L60-L67

sherlock-admin2 commented 9 months ago

Escalate

This should be of low severity, since there is a locking mechanism to ensure that previous owner cannot take any malicious action on the NFT for some duration before and after a sale.

If the lock is set right during the sale, this provides perfect protection for the buyer:

  • Seller Alice locks his NFT for an hour 30 min before the sale
  • Buyer Bob ensures NFT is locked, and checks the voting state for the NFT
  • Everything is ok, Bob buys the NFT from Alice, and has 30 mins to clean all delegations if any was left (any voting action for the NFT is locked during that time, so delegation cannot be used)

Please check: https://github.com/sherlock-audit/2023-11-convergence/blob/main/sherlock-cvg/contracts/Token/CvgERC721TimeLockingUpgradeable.sol#L60-L67

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

nevillehuang commented 9 months ago

@IAm0x52 @deadrosesxyz @10xhash you guys might want to take a look at this submission you have made.

@CergyK is making a fair point, but it requires the user to understand that they should clear their delegations. Additionally, I am unsure if there even exists a secondary market for sale of this type of NFTs.

However, even when doing simple transfers, my opinion is the delegation shouldn't be persistent, and I am also not sure if it is fair to assume any user would understand that delegation should be cleared first before such transfers unless explicitly made known to them/to watsons as a design decision during the contest.

Czar102 commented 9 months ago

I agree with the escalation, planning to consider this issue informational and accept the escalation.

it requires the user to understand that they should clear their delegations

Indeed, and the fact that users may not fully understanding the system is not a security vulnerability.

Czar102 commented 9 months ago

Result: Invalid Has duplicates

sherlock-admin2 commented 9 months ago

Escalations have been resolved successfully!

Escalation status:

walk-on-me commented 8 months ago

Hello dear lead judged :

Please checkout what we did to fix this issue here :

https://github.com/Cvg-Finance/sherlock-cvg/pull/4#discussion_r1457539133

IAm0x52 commented 8 months ago

Fix looks good. _beforeTokenTransfer now clears approvals