code-423n4 / 2022-09-frax-findings

2 stars 1 forks source link

Mistakenly use of arithmetic operation can lead to loss #329

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

Lines of code

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L40 https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L88

Vulnerability details

Mistakenly use of arithmetic operation can lead to loss

Description

Calculating the values can somewhat correct to what you need but this is a great mistake of calculation which should be overcomed .Now why should this be overcomed? Since in Solidity, the order of the arithmetic operations is not clearly defined. The operation tree’s children are evaluated in any order, but the order in which they are examined is not specified. Users can carry out various operations on operands using operators.

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L40

lib/ERC4626/src/xERC4626.sol:39:        rewardsCycleEnd = (block.timestamp.safeCastTo32() / rewardsCycleLength) * rewardsCycleLength;

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L89

lib/ERC4626/src/xERC4626.sol:89:        uint32 end = ((timestamp + rewardsCycleLength) / rewardsCycleLength) * rewardsCycleLength;

Recommendation:

Multiply before Division in Solidity: In the case of performing Integer division, Solidity may truncate the result. Hence we must multiply before dividing to prevent such loss in precision.

FortisFortuna commented 2 years ago

The truncation here is desired, so that the rewardCycleEnd goes down to the nearest integer multiple of the rewardsCycleLength.

joestakey commented 2 years ago

https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L40 This could be a duplicate of #51 , though the warden does not explain how this could lead to an exploit. https://github.com/corddry/ERC4626/blob/643cd044fac34bcbf64e1c3790a5126fec0dbec1/src/xERC4626.sol#L89 cannot be truncated as timestamp + rewardsCycleLength >= rewardsCycleLength.

0xean commented 2 years ago

invalid, low quality report.