-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
0xpep7 - CouncilMember:burn renders the contract inoperable after the first execution #199
Comments
1 comment(s) were left on this issue during the judging contest. takarez commented:
|
CouncilMember::burn
#166
See comments here for duplication reasons. |
The protocol team fixed this issue in PR/commit https://github.com/telcoin/telcoin-audit/pull/31. |
The Lead Senior Watson signed off on the fix. |
0xpep7
high
CouncilMember:burn renders the contract inoperable after the first execution
Summary
The CouncilMember contract suffers from a critical vulnerability that misaligns the balances array after a successful burn, rendering the contract inoperable.
Vulnerability Detail
The root cause of the vulnerability is that the
burn
function incorrectly manages thebalances
array, shortening it by one each time an ERC721 token is burned while the latest minted NFT still withholds its uniquetokenId
which maps to the previous value ofbalances.length
.This misalignment between existing
tokenIds
and thebalances
array results in several critical impacts:totalSupply
now collides with the existingtokenId
.This mismanagement creates a cascading effect, collectively rendering the contract inoperable. Following POC will demonstrate the issue more clearly in codes.
POC
Run
git apply
on the following patch then runnpx hardhat test
to run the POC.Result
The Passing execution of the POC confirmed that operations such as
claim
,burn
&mint
were all reverted which make the contract inoperable.Impact
The severity of the vulnerability is high due to the high likelihood of occurence and the critical impacts on the contract's operability and token holders' ability to interact with their assets.
Code Snippet
https://github.com/sherlock-audit/2024-01-telcoin/blob/main/telcoin-audit/contracts/sablier/core/CouncilMember.sol#L220
Tool used
VsCode
Recommendation
It is recommended to avoid popping out balances to keep alignment with uniquely minted tokenId. Alternatively, consider migrating to ERC1155, which inherently manages a built-in balance for each NFT.
The text was updated successfully, but these errors were encountered: