Abracadabra Mimswap - Bigsam's results

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L384-L388

Vulnerability details

Report on notifyRewardAmount Function

Impact

The existing notifyRewardAmount function is susceptible to precision loss, even when the condition if (amount < _remainingRewardTime) is satisfied. This vulnerability becomes apparent even in scenarios where the amount is substantial (e.g., 172799). The current unscaled equation (reward.rewardRate = amount / _remainingRewardTime;) results in precision loss, leading to inaccurate reward rates.

Proof of Concept

Precision Loss Scenario

1. When the condition amount < _remainingRewardTime is true, the function proceeds without reverting.
2. However, in scenarios like when the remaining time is 86400 seconds (1 day) and the amount is 172799, the equation reward.rewardRate = amount / _remainingRewardTime; results in a calculated reward rate of 1.9999884259259259, which Solidity truncates to 1, leading to a precision loss of 0.9999 plus.

Tools Used

Manual code analysis.

Recommended Mitigation Steps

Since Rewardrate has already been used in some functions consider scaling down in the two functions affected. PRECISION_FACTOR can be set to 1e18 as used in calculating rewardpertokenstored. To address the precision loss issue, it is essential to scale the equation appropriately. Update the code as follows:

1. scale up

// Scale the equation to prevent precision loss
reward.rewardRate = amount *  PRECISION_FACTOR / _remainingRewardTime;

2. scale down

function rewardsForDuration(address rewardToken) external view returns (uint256) {
        return _rewardData[rewardToken].rewardRate * rewardsDuration/ 
 PRECISION_FACTOR ;``` 

3.Remove scale because it has already been used while calculating rewardrate 
```solidity
 function _rewardPerToken(address rewardToken, uint256 lastTimeRewardApplicable_, uint256 totalSupply_) public view returns (uint256) {
        if (totalSupply_ == 0) {
            return _rewardData[rewardToken].rewardPerTokenStored;
        }

        uint256 timeElapsed = lastTimeRewardApplicable_ - _rewardData[rewardToken].lastUpdateTime;



--        uint256 pendingRewardsPerToken = (timeElapsed * _rewardData[rewardToken].rewardRate * 1e18) / totalSupply_;

++        uint256 pendingRewardsPerToken = (timeElapsed * _rewardData[rewardToken].rewardRate) / totalSupply_;



        return _rewardData[rewardToken].rewardPerTokenStored + pendingRewardsPerToken;
    } ``` 

4. earn function has already been scaled down so it can retain it's details
```solidity
 function _earned(address user, uint256 balance_, address rewardToken, uint256 rewardPerToken_) internal view returns (uint256) {
        uint256 pendingUserRewardsPerToken = rewardPerToken_ - userRewardPerTokenPaid[user][rewardToken];
        return ((balance_ * pendingUserRewardsPerToken) / 1e18) + rewards[user][rewardToken];
    }


Ensure to define `PRECISION_FACTOR` appropriately in your contract. This modification guarantees accurate reward rate calculations and eliminates precision loss concerns.``` 

By incorporating this adjustment, the `notifyRewardAmount` function will maintain precision, ensuring reliable and accurate reward rates. 





## Assessed type

Decimal

Lines of code

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L277-L286

Vulnerability details

Impact

The _rewardPerToken function, despite being only utilized for internal logic and calculations within the contract, has been labeled as a public function. This exposes it to external calls and inputs, potentially leading to unexpected contract behaviors. Given the function's critical role in calculating the sensitive value REWARDPERTOKEN for fairness and reward distribution, this exposure poses a security risk.

    function _rewardPerToken(address rewardToken, uint256 lastTimeRewardApplicable_, uint256 totalSupply_) public view returns (uint256) {
        if (totalSupply_ == 0) {
            return _rewardData[rewardToken].rewardPerTokenStored;
        }

        uint256 timeElapsed = lastTimeRewardApplicable_ - _rewardData[rewardToken].lastUpdateTime;
        uint256 pendingRewardsPerToken = (timeElapsed * _rewardData[rewardToken].rewardRate * 1e18) / totalSupply_;

        return _rewardData[rewardToken].rewardPerTokenStored + pendingRewardsPerToken;
    }

Proof of Concept

1. The function is currently declared as public, allowing external entities to call it.
2. External calls to this function could manipulate the input logic and inaccurately calculate REWARDPERTOKEN by calling the function multiple times with wrong inputs.

Code Reference

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L277-L286

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L528-L534

https://github.com/code-423n4/2024-03-abracadabra-money/blob/1f4693fdbf33e9ad28132643e2d6f7635834c6c6/src/staking/LockingMultiRewards.sol#L273-L275

Tools Used

Manual code analysis.

Recommended Mitigation Steps

To enhance the security of the contract and protect the critical internal logic implemented in the _rewardPerToken function, it is strongly recommended to change the function's visibility to internal. This adjustment ensures that the function can only be called within the current contract or contracts that inherit from it.

Update the function declaration as follows:

function _rewardPerToken(address rewardToken, uint256 lastTimeRewardApplicable_, uint256 totalSupply_) internal view returns (uint256) {
   
}

By making this adjustment, you limit external access to the _rewardPerToken function, reducing the risk of unintended manipulation and ensuring the integrity of the sensitive calculations it performs.

Assessed type

Error