OKC Swap

In the contract `StakingRewards`, the role `rewardsDistribution` has authority over the functions shown in the diagram below.
Any compromise to the `rewardsDistribution` account may allow the hacker to take advantage of this authority and change the amount of rewards or the reward duration.
![](https://accelerator-tasks-prod.acc.corp.certik.com/009df8265b654454a8173f122fa7002e/diagrams/centralization_StakingRewards-StakingRewards-rewardsDistribution.svg)

In the contract `StakingRewardsFactory`, the role `_owner` has authority over the functions shown in the diagram below.
Any compromise to the `_owner` account may allow the hacker to take advantage of this authority and change the amount of rewards or the reward duration for staking contracts.
![](https://accelerator-tasks-prod.acc.corp.certik.com/009df8265b654454a8173f122fa7002e/diagrams/centralization_StakingRewardsFactory-StakingRewardsFactory-_owner.svg)

In the contract `OKCSwapFactory`, the role `feeToSetter` has authority over the functions shown in the diagram below.
Any compromise to the `feeToSetter` account may allow the hacker to take advantage of this authority and change the amount of liquidity fees as well as change the fee recipient.
![](https://accelerator-tasks-prod.acc.corp.certik.com/009df8265b654454a8173f122fa7002e/diagrams/centralization_OKCSwapFactory-OKCSwapFactory-feeToSetter.svg)

In the contract `OKCSwapPair` the role `factory` has authority over the functions shown in the diagram below.
Any compromise to the `factory` account may allow the hacker to take advantage of this authority and initialize the incorrect tokens for the pair.
![](https://accelerator-tasks-prod.acc.corp.certik.com/009df8265b654454a8173f122fa7002e/diagrams/centralization_OKCSwapPair-OKCSwapPair-factory.svg)

When transferring deflationary ERC20 tokens, the input amount may not be equal to the received amount due to the charged transaction fee. For example, if a user sends 100 deflationary tokens (with a 10% transaction fee), only 90 tokens actually arrived to the contract. However, a failure to discount such fees may allow the same user to withdraw 100 tokens from the contract, which causes the contract to lose 10 tokens in such a transaction.

Reference:
https://thoreum-finance.medium.com/what-exploit-happened-today-for-gocerberus-and-garuda-also-for-lokum-ybear-piggy-caramelswap-3943ee23a39f

```solidity=90
        stakingToken.safeTransferFrom(msg.sender, address(this), amount);
```
- Transferring tokens by `amount`.

```solidity=85
        _balances[msg.sender] = _balances[msg.sender].add(amount);
```
- The `amount` is used for bookkeeping purposes without compensating for potential transfer fees.


---
```solidity=98
        stakingToken.safeTransferFrom(msg.sender, address(this), amount);
```
- Transferring tokens by `amount`.

```solidity=97
        _balances[msg.sender] = _balances[msg.sender].add(amount);
```
- The `amount` is used for bookkeeping purposes without compensating for potential transfer fees.


---
```solidity=45
            require(
                rewardsToken.transfer(stakingRewards, rewardAmount),
                'StakingRewardsFactory::notifyRewardAmount: transfer failed'
            );
```
- Transferring tokens by `rewardAmount`.

```solidity=49
            StakingRewards(stakingRewards).notifyRewardAmount(rewardAmount,rewardsDuration);
```
- The `rewardAmount` is used for bookkeeping purposes without compensating for potential transfer fees.

### Proof of Concept

The following unit test created using `forge` demonstrates the issue by performing the following:
1. A deflationary ERC20 token is created where when transferring tokens, only 90% of the inputted amount is transferred
2. The test mints 100 tokens to the contract and the contract stakes 100 tokens
3. The staking balance is compared with ERC20 balance of the staking contract
4. The test fails as expected as the staking balance is 100 while the ERC20 balance is 90

```solidity
pragma solidity 0.6.12;
pragma experimental ABIEncoderV2;

import "forge-std/Test.sol";
import {mockERC20} from "../src/mocks/mockERC20.sol";
import {mockWETH} from "../src/mocks/mockWETH.sol";
import {StakingRewards} from "../src/farm/StakingRewards.sol";

contract DeflationaryERC20 is mockERC20 {
    function _transfer(address sender, address recipient, uint256 amount) internal override {
        amount = amount * 9 / 10;
        super._transfer(sender, recipient, amount);
    }
}

contract StakingRewardsTest is Test {
    StakingRewards public stakingRewards;
    DeflationaryERC20 public mockToken;
    mockWETH public WETH;

    function setUp() public {
        mockToken = new DeflationaryERC20();
        WETH = new mockWETH();
        stakingRewards = new StakingRewards(address(mockToken), address(WETH));
    }

    function testStakeDeflationary() public {
        mockToken.mint(address(this), 100);
        mockToken.approve(address(stakingRewards), 100);
        stakingRewards.stake(100);
        uint256 balance = stakingRewards.balanceOf(address(this));
        uint256 actualBalance = mockToken.balanceOf(address(stakingRewards));

        assertFalse(actualBalance == balance);
    }
}

```

It is expected that calls of the form `approve(spender, amount)` fail if the address in `spender` is the zero address.

It is expected that calls of the form 
- `transfer(recipient, amount)`
- `transferFrom(from, dest, amount)`

fail if the recipient address is the zero address.

When there are no stakers, the total staked amount is 0. Then when updating rewards, only the reward update time is updated.

```solidity=165
    modifier updateReward(address account) {
        rewardPerTokenStored = rewardPerToken();
        lastUpdateTime = lastTimeRewardApplicable();
```
```solidity=62
    function rewardPerToken() public view override returns (uint256) {
        if (_totalSupply == 0) {
            return rewardPerTokenStored;
        }
```

If the duration of time for when there are no stakers is denoted as `T`, then `rewardRate * T` rewards will be forever locked in the contract. This is because when updating the reward amount, the contract does not handle rewards that have not been earned due to no stakers.

```solidity=135
    function notifyRewardAmount(uint256 reward, uint256 newRewardsDuration) external override onlyRewardsDistribution updateReward(address(0)) {
        rewardsDuration = newRewardsDuration;
        if (block.timestamp >= periodFinish) {
            rewardRate = reward.div(rewardsDuration);
        } else {
            uint256 remaining = periodFinish.sub(block.timestamp);
            uint256 leftover = remaining.mul(rewardRate);
            rewardRate = reward.add(leftover).div(rewardsDuration);
        }
```

For example, suppose the first reward duration was for 10 days and 100 tokens are given as rewards each day. If there are no stakers for the first 5 days and the first staker stakes on the 6th day, then the 500 tokens meant to be given as rewards will be locked inside the contract forever.

When adding rewards to the staking contract, a check is performed to ensure the contract's balance is sufficient for the stated reward.

```solidity=149
        uint balance = rewardsToken.balanceOf(address(this));
        require(rewardRate <= balance.div(rewardsDuration), "Provided reward too high");
```

However, in the situation where the reward token is the same as the staking token, this check can be bypassed as `balance` will include tokens that users have staked.

### Proof of Concept

The below test done in forge does the following:
1. A staking contract is created where the staking token and reward token are the same
2. 100 tokens are staked to the contract
3. `notifyRewardAmount()` is called and succeeds, stating 100 reward tokens are available even though no reward tokens were transferred to the contract

```solidity
pragma solidity 0.6.12;
pragma experimental ABIEncoderV2;

import "forge-std/Test.sol";
import {mockERC20} from "../src/mocks/mockERC20.sol";
import {StakingRewards} from "../src/farm/StakingRewards.sol";

contract StakingRewardsTest is Test {
    StakingRewards public stakingRewards;
    mockERC20 public mockToken;

    function setUp() public {
        mockToken = new mockERC20();
        stakingRewards = new StakingRewards(address(mockToken), address(mockToken));
    }

    function testBypassRewardCheck() public {
        mockToken.mint(address(this), 100);
        mockToken.approve(address(stakingRewards), 100);
        stakingRewards.stake(100);
        stakingRewards.notifyRewardAmount(100, 1);
    }
}
```

The signature malleability is possible within the Elliptic Curve cryptographic system. An Elliptic Curve is symmetric on the X-axis, meaning two points can exist with the same `X` value. In the `r`, `s` and `v` representation this permits us to carefully adjust `s` to produce a second valid signature for the same `r`, thus breaking the assumption that a signature cannot be replayed in what is known as a replay-attack.

### Proof of Concept

Using the signature obtained from https://solidity-by-example.org/signature/, the below unit test in `forge` shows that two different `s` and `v` values can produce the same address for one `r` value.

```solidity
pragma solidity 0.6.12;
pragma experimental ABIEncoderV2;

import "forge-std/Test.sol";

contract ECRecoverTest is Test {
    function setUp() public {
    }

    function getMessageHash(
        address _to,
        uint _amount,
        string memory _message,
        uint _nonce
    ) public pure returns (bytes32) {
        return keccak256(abi.encodePacked(_to, _amount, _message, _nonce));
    }

    function getEthSignedMessageHash(bytes32 _messageHash)
        public
        pure
        returns (bytes32)
    {
        return
            keccak256(
                abi.encodePacked("\x19Ethereum Signed Message:\n32", _messageHash)
            );
    }

    function testVerifySignature() public {
        bytes32 r = 0x993DAB3DD91F5C6DC28E17439BE475478F5635C92A56E17E82349D3FB2F16619;
        bytes32 s_a = 0x6F466C0B4E0C146F285204F0DCB13E5AE67BC33F4B888EC32DFE0A063E8F3F78;
        bytes32 s_b = 0x90B993F4B1F3EB90D7ADFB0F234EC1A3D43319A763C0117891D4548691A701C9;
        uint8 v_a = 0x1B; // 27
        uint8 v_b = 0x1C; // 28
        
        bytes32 messageHash = getMessageHash(0x14723A09ACff6D2A60DcdF7aA4AFf308FDDC160C, 123, "coffee and donuts", 1);
        bytes32 ethSignedMessageHash = getEthSignedMessageHash(messageHash);

        address owner_a = ecrecover(ethSignedMessageHash, v_a, r, s_a);
        address owner_b = ecrecover(ethSignedMessageHash, v_b, r, s_b);
        assertEq(owner_a, owner_b);
    }
```

The following interface is unused:
```solidity=4
import '../interfaces/IOKCSwapPair.sol';
```

The linked variables assigned in the constructor can be declared as `immutable`. Immutable state variables can be assigned during contract creation but will remain constant throughout the lifetime of a deployed contract. A big advantage of immutable variables is that reading them is significantly cheaper than reading from regular state variables since they will not be stored in storage.