sherlock-audit / 2024-02-telcoin-platform-audit-update-judging

3 stars 1 forks source link

0xkmg - Function in StablecoinHandler may revert on underflow and malfunction the contract #58

Closed sherlock-admin2 closed 7 months ago

sherlock-admin2 commented 7 months ago

0xkmg

medium

Function in StablecoinHandler may revert on underflow and malfunction the contract

Summary

Some functions in both StablecoinHandler and the children contract implemented can revert on underflow.

Vulnerability Detail

Impact

An underflow condition can disrupt normal contract operations, leading to unexpected reverts that mask the intended error messages. Specifically, calculations that subtract ss.oAmount from Stablecoin(ss.origin).totalSupply() without prior validation can fail silently if totalSupply is less than ss.oAmount, resulting in a generic underflow error rather than a more descriptive and anticipated revert. This not only makes debugging difficult but also obscures the logic's intent, potentially complicating error handling and interaction patterns with the contract.

   if (Stablecoin(ss.origin).totalSupply() - ss.oAmount < getMinLimit(ss.origin)) revert InvalidMintBurnBoundry(ss.origin);

Code Snippet

https://github.com/sherlock-audit/2024-02-telcoin-platform-audit-update/blob/main/telcoin-contracts/contracts/stablecoin/StablecoinHandler.sol#L120-L123

https://github.com/sherlock-audit/2024-02-telcoin-platform-audit-update/blob/main/telcoin-contracts/contracts/stablecoin/StablecoinHandler.sol#L171-L174

https://github.com/sherlock-audit/2024-02-telcoin-platform-audit-update/blob/main/telcoin-contracts/contracts/swap/AmirX.sol#L89-L92

Tool used

Manual Review

Recommendation

To mitigate this issue and enhance error transparency, it is recommended to first validate that the total supply is greater than the amount to be subtracted. This approach ensures that the operation can only proceed if it will not result in an underflow, thereby preserving the intended logic flow and making errors more understandable:

 if (Stablecoin(ss.origin).totalSupply() > ss.oAmount) {
            if (Stablecoin(ss.origin).totalSupply() - ss.oAmount < getMinLimit(ss.origin)) revert InvalidMintBurnBoundry(ss.origin);
        }

This preventive measure not only avoids underflow but also clarifies the contract's behavior, making it more predictable and easier to interact with safely.

sherlock-admin2 commented 7 months ago

1 comment(s) were left on this issue during the judging contest.

WangAudit commented:

not described how can we end up in such scenario and seems that impact is the DOS of one swap; so it's against the rules