OpenLeverage contest - csanuragjain's results

Handle

csanuragjain

Vulnerability details

Impact

User funds can be lost if Admin sets startTimes[i] to 0

Proof of Concept

1. Navigate to contract https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/farming/FarmingPools.sol

2. Check the initDistributions function

function initDistributions(address[] memory stakeTokens, uint64[] memory startTimes, uint64[] memory durations) external onlyAdmin {
        for (uint256 i = 0; i < stakeTokens.length; i++) {
            require(distributions[stakeTokens[i]].starttime == 0, 'Init once');
            distributions[stakeTokens[i]] = Distribution(durations[i], startTimes[i], 0, 0, 0, 0, 0);
        }
    }

3. Assume Admin calls this for token X with startTimes[i] as 0. This creates a new distribution with start time as 0 for token X

4. User Y stakes amount 500 for this token X

5. Admin calls initDistributions again with token X and startTimes[i] as 1000. This overwrites and reinitializes distributions[X] which means totalStaked becomes 0 and contract has lost all track of user funds now

Recommended Mitigation Steps

Add a check to see startTimes[i]!=0 in initDistributions function

Handle

csanuragjain

Vulnerability details

Impact

If contract does not have enough reward token, full user reward are stuck. Their is no provision for partial reward payment

Proof of Concept

1. Navigate to https://github.com/code-423n4/2022-01-openleverage/blob/main/openleverage-contracts/contracts/farming/FarmingPools.sol

2. Observe the getReward function

function getReward(address stakeToken) public updateReward(stakeToken, msg.sender) checkStart(stakeToken) {
        uint256 reward = rewards[stakeToken][msg.sender].rewards;
        if (reward > 0) {
            rewards[stakeToken][msg.sender].rewards = 0;
            oleToken.safeTransfer(msg.sender, reward);
            emit RewardPaid(stakeToken, msg.sender, reward);
        }
    }

3. As we can see if user reward are 500 and contract has only 499 rewards left then no reward will be transferred to the user. User is blocked until contract adds more rewards. Instead contract could have atleast transferred 499 rewards and kept 1 as leftover reward in rewards[stakeToken][msg.sender].rewards

Recommended Mitigation Steps

if reward balance is less contract must transfer whatever amount is left in contract and rest can be recorded in user leftover rewards

Handle

csanuragjain

Vulnerability details

Impact

Gas savings and minor corrections

Proof of Concept

1. Adminable.sol - developer can be immutable

2. OLEToken.sol, ControllerDelegator.sol, OpenLevDelegator.sol, XOLEDelegator.sol, DexAggregatorDelegator.sol - admin can be immutable

3. Reserve.sol - admin & oleToken can be immutable

4. Timelock.sol - In queueTransaction function, Add check to see require(!queuedTransactions[txHash], "Already queued")

5. OLETokenLock.sol - In constructor, beneficiaries.length should be stored locally and then use for gas saving. Also check startTimes[i]!=endTimes[i]

6. FarmingPools.sol - a. In notifyRewardAmounts function, check stakeTokens[i]== reward[i] b. In initDistributions function, length should be same for stakeTokens, startTimes and durations c. rewardPerToken function, assert(lastTimeRewardApplicable >= distribution.lastUpdateTime); could be changed to assert(lastTimeRewardApplicable > distribution.lastUpdateTime); since if lastTimeRewardApplicable = distribution.lastUpdateTime then change in reward will be 0