ast3ros - In case of stock split and reverse split, the Dshare token holder will gain or loss his Dshare token value

[sherlock-admin]

ast3ros

high

In case of stock split and reverse split, the Dshare token holder will gain or loss his Dshare token value

Summary

Stock split and reverse split may cause the token accounting to be inaccurate.

Vulnerability Detail

Stock split and reverse split are very common in the stock market. There are many examples here: https://companiesmarketcap.com/amazon/stock-splits/

For instance, in a 2-for-1 stock split, a shareholder receives an additional share for each share held. However, the DShare token holder still holds only one DShare token after the split. If a DShare token holder owns 100 DShare tokens before the split, he will still own 100 DShare tokens after the split. However, he should own 200 DShare tokens after the split.

Currently, users can buy 1 DShare token at the current market price of the underlying share. https://sbt.dinari.com/tokens

This means that after the stock split, a new Dshare token holder can buy a Dshare at half the price of the previous Dshare token holder. This is unfair to the previous Dshare token holder. In other words, the original Dshare token holder will lose 50% of his Dshare token value after the stock split.

The same logic applies to stock reverse split.

Impact

The Dshare token holder will gain or loss his Dshare token value after the stock split or reverse split.

Code Snippet

https://github.com/sherlock-audit/2023-06-dinari/blob/50eb49260ab54e02748c2f6382fd95284d271f06/sbt-contracts/src/BridgedERC20.sol#L13

Tool used

Manual Review

Recommendation

The Operator should have a mechanism to mint or burn DShare tokens of holders when the underlying share is split or reverse split.

[jaketimothy]

While this would be catastrophic if unaddressed, our current - admittedly undocumented - approach is to halt and cancel all orders for assets at the end of the day the split is announced. Then deploy a new token with a burn->mint migration contract at the appropriate split ratio, add the new token to the issuers, and re-enable orders with the new token.

This accounts for various scenarios where the original tokens may be locked in other contracts without Dinari having to push airdrop transactions for every split - or maintaining burn privileges on all owners/holders.

[ctf-sec]

The amazon last stock split is 20 years ago. I think this report is out of scope

The Operator should have a mechanism to mint or burn DShare tokens of holders when the underlying share is split or reverse split.

The operator is a privileged central party that can do that.

[Oot2k]

I still think this issue is valid. Stock splits happen fairly often, for example Tesla in 2020 and 2022. I think this is a valid medium, because the offchain system was not mentioned anywhere and there still can be issues when deploying a token manually. There wont be direct loss of funds, but protocols that integrate with dinari will have problems (Dex, lending etc.)

[ctf-sec]

While the stock and offline logic is out of scope,

In 2-for-1 stock split, a shareholder receives an additional share for each share held

In the current implementation, this would require the admin mint stock for user

In a stock reverse split,

In the current implementation, admin can't burn stock for user

  function burn(uint256 value) external virtual onlyRole(BURNER_ROLE) {
        _burn(msg.sender, value);
    }

if the implementation is

  function burn(address from, uint256 value) external virtual onlyRole(BURNER_ROLE) {
        _burn(from, value);
    }

I would agree this is a low severity and out of scope finding,

but since the admin can't actually burn for user,

this can be valid medium, severity is definitely not high :)

[thangtranth]

Escalate

I believe this issue is a high severity because:

• Impact: As the sponsor mentioned, the impact is catastrophic if unaddressed. It breaks the invariant 1 share : 1 token of the protocol. Some token holders will unfairly lose/gain X times the value. In addition, protocols that integrate with Dinari will have problems. It is not considered an acceptable risk

• Frequency: Stock splits and reverse splits are very common events. Because Dinari covers many publicly traded securities, not just one stock, the frequency of the events should be counted using the whole stock market, not frequency of one stock. There are events happening every month. https://www.marketbeat.com/stock-splits/history/

• The mechanism that the sponsor mentioned was not available anywhere during the contest: from the contest README to the white paper. It is new information that came after the contest was finished.

• As the lead Watson pointed out, even the workaround of owner minting and burning directly to users does not work, because the admin cannot burn other users’ tokens. And I don’t think using the mint and burn functions in the token contract and minting manually to each user is a good solution.

[sherlock-admin]

Escalate

I believe this issue is a high severity because:

• Impact: As the sponsor mentioned, the impact is catastrophic if unaddressed. It breaks the invariant 1 share : 1 token of the protocol. Some token holders will unfairly lose/gain X times the value. In addition, protocols that integrate with Dinari will have problems. It is not considered an acceptable risk

• Frequency: Stock splits and reverse splits are very common events. Because Dinari covers many publicly traded securities, not just one stock, the frequency of the events should be counted using the whole stock market, not frequency of one stock. There are events happening every month. https://www.marketbeat.com/stock-splits/history/

• The mechanism that the sponsor mentioned was not available anywhere during the contest: from the contest README to the white paper. It is new information that came after the contest was finished.

• As the lead Watson pointed out, even the workaround of owner minting and burning directly to users does not work, because the admin cannot burn other users’ tokens. And I don’t think using the mint and burn functions in the token contract and minting manually to each user is a good solution.

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.

[ctf-sec]

Escalate

I believe this issue is a high severity because:

• Impact: As the sponsor mentioned, the impact is catastrophic if unaddressed. It breaks the invariant 1 share : 1 token of the protocol. Some token holders will unfairly lose/gain X times the value. In addition, protocols that integrate with Dinari will have problems. It is not considered an acceptable risk
• Frequency: Stock splits and reverse splits are very common events. Because Dinari covers many publicly traded securities, not just one stock, the frequency of the events should be counted using the whole stock market, not frequency of one stock. There are events happening every month. https://www.marketbeat.com/stock-splits/history/
• The mechanism that the sponsor mentioned was not available anywhere during the contest: from the contest README to the white paper. It is new information that came after the contest was finished.
• As the lead Watson pointed out, even the workaround of owner minting and burning directly to users does not work, because the admin cannot burn other users’ tokens. And I don’t think using the mint and burn functions in the token contract and minting manually to each user is a good solution.

I find it difficult to think this is a high severity issue, stock just don't split every day. How does the sponsor handle the stock split is not in scope of auditing, severity at most medium if not out of scope, we could say, if the stock company rug and delisted, all dShare lose its value... this is expected, which is not in scope of the auditing as well

https://docs.sherlock.xyz/audits/judging/judging

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future.

and

As the lead Watson pointed out, even the workaround of owner minting and burning directly to users does not work, because the admin cannot burn other users’ tokens. And I don’t think using the mint and burn functions in the token contract and minting manually to each user is a good solution

there is no mention about this in the original report

and

I really feel like not dispute as low and out of scope and argue a medium is already what I can do the best for this report because this report does show creativity.

https://www.marketplace.org/2022/02/11/whats-a-stock-split-anyway/

[ctf-sec]

btw the while the resolving the escalation is one story, the fix is another.

@jaketimothy

Maybe can use this

https://docs.openzeppelin.com/contracts/3.x/api/token/erc20#ERC20Snapshot

Implement as rebasing token to modify the ERC20 token balance can help as well!

Agree this is not a easy fix.

[jaketimothy]

Thank you for sharing @ctf-sec. I'm racking my brain trying to avoid rebasing token.

[Oot2k]

Stocksplits are known well in advance, so trading can be haltet and shares converted. I still think all offchain fixes are not recommended which means this is a valid medium issue.

[jaketimothy]

Fixed in

• https://github.com/dinaricrypto/sbt-contracts/pull/135

[hrishibhat]

Result: Medium Unique Given that this is an external condition that is well-known beforehand. This issue can be fairly considered a valid medium because the code cannot handle the stock-split situation and the off-chain/on-chain solutions were not previously present.

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• thangtranth: rejected