search

code-423n4 / 2022-06-nibbl-findings

1 stars 0 forks source link

Some functions and return values might revert due to overflow #263

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L195 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L253

Vulnerability details

Impact

NibblVault contract's initialize() function has the calculation as below;

195:         uint _primaryReserveBalance = (primaryReserveRatio * _initialTokenSupply * _initialTokenPrice) / (SCALE * 1e18);

The params are uint32 primaryReserveRatio , uint256 _initialTokenSupply, uint256 _initialTokenPrice respectively. Since there are no limits of input parameters during calling this function, final calculation can fit into result data type uint256 but some intermediate operations may overflow. This phenomenon is described here with the name of Phantom overflows.

Proof of Concept

https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L195 https://github.com/code-423n4/2022-06-nibbl/blob/8c3dbd6adf350f35c58b31723d42117765644110/contracts/NibblVault.sol#L253

Tools Used

Manual Review

Recommended Mitigation Steps

A constant like SCALE can be utilized again to refactor the formula and avoid this.

HardlyDifficult commented 2 years ago

Agree that a phantom overflow might apply, but given the overflow in question happens in the initializer then a revert is not harmful. The params could be adjusted and then they could try creating the vault again.

If inputs may be large enough to overflow here then the recommendation is a good consideration. Merging this with the warden's QA report #259