As a result of operator precedence (division is done before multiplication, and multiplication before addition and subtraction), use of paranthesis is a must to avoid loss of precision. This is because if two values are divided first and are rounded down in case of incomplete division (solidity default behaviour), the final value (after multiplication and other operations) is lesser than what it would have been had all other requisite operations been done before division (as per the calculation requirements).
Following are explanations of the issues where there were found in the repository.
BondNFT
In createLock, the share calculation is done in the follwing manner:
uint shares = _amount * _period / 365
Since the division will be done first, and given that solidity always rounds down, there is loss of precision here. This leads to a lower share amount for a given lock amount.
It is recommended to change the share calculation so the multiplication happens first:
uint shares = (_amount * _period) / 365
In extendLock(), the shares calculation uint shares = (bond.amount + _amount) * (bond.period + _period) / 365; loses precision by dividing the second parentheses first and then multipliying th result by the first parentheses. This causes the shares be a lower number.
Instead, it should changed so that all multiplication is done before the division:
In claim(_id, _claimer), the calculation for incrementing accRewardsPerShare loses precision by dividing 1e18 by totalShares[bond.asset] first. Instead, it should be changed to the following:
Here, the amount calculated is ultimately smaller due to the divising and rounding down first, and then the shares being miltiplied by the result. This leads to a lesser bond.pending (pending rewards) amount being reported than it actually is. Other functions/ external protocols depending on this function would therefore get a false picture of the user bond. This should be changed to prioritise mutiplication before division:
In lzReceive(_srcChainId, _srcAddress, _nonce, _payload), in the excessivelySafeCall call, consider the following arithmetic which calculates the gas to be sent in the call:
[...]gasleft()*4/5, 150,[...]
Since solidity rounds down by defauly, 4/5 = 0.8 which rounded down is 0. This means, that the gas sent in the call will always be 0, leading to the excessivelySafeCall always failing, and this function becoming unusable.
It is recommended to change the arithmetic so that the multiplication is done before the division by 5:
[...](gasleft()*4)/5, 150,[...]
Proof of Concept
Use calculations without regard for operator precision.
Incur precision loss, leading to calculations being smaller.
Result in loss to the user and/or the protocol over time.
Lines of code
https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L65 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L113 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L122 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L180 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L225 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/BondNFT.sol#L241 https://github.com/code-423n4/2022-12-tigris/blob/main/contracts/GovNFT.sol#L175
Vulnerability details
Impact
As a result of operator precedence (division is done before multiplication, and multiplication before addition and subtraction), use of paranthesis is a must to avoid loss of precision. This is because if two values are divided first and are rounded down in case of incomplete division (solidity default behaviour), the final value (after multiplication and other operations) is lesser than what it would have been had all other requisite operations been done before division (as per the calculation requirements).
Following are explanations of the issues where there were found in the repository.
BondNFT
createLock
, the share calculation is done in the follwing manner:uint shares = _amount * _period / 365
Since the division will be done first, and given that solidity always rounds down, there is loss of precision here. This leads to a lower share amount for a given lock amount.
It is recommended to change the share calculation so the multiplication happens first:
uint shares = (_amount * _period) / 365
extendLock()
, the shares calculationuint shares = (bond.amount + _amount) * (bond.period + _period) / 365;
loses precision by dividing the second parentheses first and then multipliying th result by the first parentheses. This causes the shares be a lower number.Instead, it should changed so that all multiplication is done before the division:
uint shares = ((bond.amount + _amount) * (bond.period + _period)) / 365;
Similarly, the
bondPaid
calculation should be chenged likewise:bondPaid[_id][bond.asset] = (accRewardsPerShare[bond.asset][epoch[bond.asset]] * _bond.shares) / 1e18;
claim(_id, _claimer)
, the calculation for incrementingaccRewardsPerShare
loses precision by dividing 1e18 by totalShares[bond.asset] first. Instead, it should be changed to the following:accRewardsPerShare[bond.asset][epoch[bond.asset]] += (_pendingDelta*1e18)/totalShares[bond.asset];
distribute(_tigAsset, _amount)
, on this line:accRewardsPerShare[_tigAsset][aEpoch] += _amount * 1e18 / totalShares[_tigAsset];
The accRewardsPerShare is incremented by a lower share amount due to loss of precision and rounding down. This should be changed to:
accRewardsPerShare[_tigAsset][aEpoch] += (_amount * 1e18) / totalShares[_tigAsset];
idToBond(_id)
, the calculation:bond.pending = bond.shares * _accRewardsPerShare / 1e18 - bondPaid[_id][bond.asset];
Here, the amount calculated is ultimately smaller due to the divising and rounding down first, and then the shares being miltiplied by the result. This leads to a lesser
bond.pending
(pending rewards) amount being reported than it actually is. Other functions/ external protocols depending on this function would therefore get a false picture of the user bond. This should be changed to prioritise mutiplication before division:bond.pending = (bond.shares * _accRewardsPerShare) / 1e18 - bondPaid[_id][bond.asset];
GovNFT.sol
lzReceive(_srcChainId, _srcAddress, _nonce, _payload)
, in theexcessivelySafeCall
call, consider the following arithmetic which calculates the gas to be sent in the call:[...]gasleft()*4/5, 150,[...]
Since solidity rounds down by defauly,4/5 = 0.8
which rounded down is0
. This means, that the gas sent in the call will always be 0, leading to theexcessivelySafeCall
always failing, and this function becoming unusable.It is recommended to change the arithmetic so that the multiplication is done before the division by 5:
[...](gasleft()*4)/5, 150,[...]
Proof of Concept
Tools Used
Recommended Mitigation Steps