code-423n4 / 2022-12-tigris-findings

8 stars 4 forks source link

:in stablevault.sol withdraw function there is no check to ensure value returned by `IERC20Mintable(_token).decimals()` Is not 0 #601

Closed code423n4 closed 1 year ago

code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2022-12-tigris/blob/588c84b7bb354d20cbca6034544c4faa46e6a80e/contracts/StableVault.sol#L67

Vulnerability details

Impact

_output = _amount/10**(18-IERC20Mintable(_token).decimals()) line. If the IERC20Mintable(_token).decimals() function returns 0, this line will attempt to divide by 0, which could cause the code to throw an error or behave in an unintended manner.

Proof of Concept

`_output = _amount/10**(18-IERC20Mintable(_token).decimals())

Tools Used

manually

Recommended Mitigation Steps

To address the potential divide-by-zero error in this code, it would be recommended to add a check to ensure that the value returned by IERC20Mintable(_token).decimals() is not 0 before attempting to divide by it. This could be done by adding an if statement to check the value, and either returning an error or using a different calculation method if the value is 0. For example:


`function withdraw(address _token, uint256 _amount) external returns (uint256 _output) {     IERC20Mintable(stable).burnFrom(_msgSender(), _amount);  

if (IERC20Mintable(_token).decimals() == 0) {         
// Return an error or use a different calculation         
// method to avoid dividing by 0        
return 0;     }  

_output = _amount/10**(18-IERC20Mintable(_token).decimals());     

IERC20(_token).transfer(         _msgSender(),         _output     ); }`

This would prevent the code from attempting to divide by 0, which would eliminate the potential vulnerability.
GalloDaSballo commented 1 year ago

This will most likely be QA:

GalloDaSballo commented 1 year ago

I also believe the tx will revert immediately so funds will not be at risk

GalloDaSballo commented 1 year ago

L

c4-judge commented 1 year ago

GalloDaSballo changed the severity to QA (Quality Assurance)

c4-sponsor commented 1 year ago

GainsGoblin marked the issue as sponsor confirmed

GalloDaSballo commented 1 year ago

1L from Dups

2L

c4-judge commented 1 year ago

GalloDaSballo marked the issue as grade-c