Sign Up for a Code Audit Today and Get 3 Months of Skynet for Free

The functions which are never called internally within the contract should have external visibility for gas optimization.

Function Visibility Optimization

There should always be events emitted in the sensitive functions that are controlled by centralization roles.

Missing Emit Events

The return value of the linked `transferFrom()` call is not handled.

Unchecked ERC-20 `transferFrom()` Call

In the contract `FarmController` the role `_whitelistAdmins` has authority over the following functions:
- `FarmController.add()`, which adds a liquidity pool;
- `FarmController.set()`, which changes a pool's allocation points;
- `WhitelistAdminRole.addWhitelistAdmin()`, which gives an account the `_whitelistAdmins` role.

Any compromise to a `_whitelistAdmins` account may allow the hacker to take advantage of this authority and add a malicious pool or prevent rewards from a pool.

Centralization Risk in `FarmController.sol`

A user's rewards are determined by their working supply which is in turn determined by a user's deposit amount, the pool's total supply of deposited tokens, and the balance of the user in the `VotingEscrow` contract. 
```solidity=223
        uint256 l = (k * user.amount) / 100;
        uint256 votingTotal = votingEscrow.totalSupply();
        if (votingTotal > 0)
            l +=
                (((pool.totalSupply * votingEscrow.balanceOf(_user)) /
                    votingTotal) * (100 - k)) /
                100;
        if (l > user.amount) l = user.amount;
        pool.workingSupply = pool.workingSupply + l - user.workingSupply;
        user.workingSupply = l;
```

A user can therefore use two accounts to obtain a larger working supply than they should have by having one account deposit tokens to inflate `pool.totalSupply` and the other account with a positive `votingEscrow` balance to deposit tokens to acquire a higher working supply. The user can then withdraw the tokens used to inflate `pool.totalSupply`, resulting in acquiring more rewards than expected. The user also cannot be kicked as their balance in `votingEscrow` will be positive.

This is a greater concern if a malicious contract is whitelisted in the `VotingEscrow` contract, as then a flash loan attack can be used to inflate `pool.totalSupply` and the malicious contract that is able to create a lock for itself would be able to increase the amount of rewards it obtains through `deposit()`.

Working Supply Can Be Manipulated

In the `add()` and `set()` functions, the flag '_withUpdate' determines if all pools will be updated. This reliance might lead to a significant loss of the reward amount.

```solidity
if (_withUpdate) {
    massUpdatePools();
}
```

For an illustration, assume we only have one pool and it has the values `pool.allocPoint == 50` and `totalAllocPoint == 50`. Now we want to add another pool with `pool.allocPoint == 50`.

There will be two scenarios for calculating the pool reward:

Case 1: `withUpdate` is set to `true`.
- Distribute the reward and update the pool.
- Add the given pool information.

Case 2: `withUpdate` is set to `false`:
- Add the given pool information.
 
(Note: While we focused on the `add()` function, both the `add()` and `set()` functions update `totalAllocPoint`, which is used in the calculation of pool rewards in the function `updatePool()`)

In Case 1, the reward for the first pool is updated in the call to `updatePool()`:
```solidity=184
        uint256 reward =
            (PPIRate(ppiRate).calculateReward(
                pool.lastRewardTime,
                block.timestamp
            ) * pool.allocPoint) / totalAllocPoint;
```
so we have `reward = ppiRate.calculateReward() * 50 / 50 = multiplier * UPPerBlock`.

In Case 2, an update `totalAllocPoint = totalAllocPoint.add(_allocPoint)` is done first. Then when `updatePool()` is called later, the calculation of the reward for the first pool becomes `reward = multiplier * ppiRate.calculateReward() * 50 / 100`, half of what it is expected to be.

Potential Loss of Pool Rewards

The function `initialize()` sets important variables such as addresses to receive fees, the contracts that `FarmController` depends on, and a whitelist administrator of the contract. As such, there should be access control to ensure that an attacker does not initialize the contract.

Initialization of `FarmController` Lacks Restrictions

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

The contract also takes the pool token balance (the `workingSupply`) into account when calculating users' rewards. An attacker can repeat the process of deposit and withdraw to lower the token balance in a deflationary token pool and cause the contract to increase the reward amount. 

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

Incompatibility With Deflationary Tokens(Farming)

The function `calculateReward()` is used to calculate the rewards for pools in the `FarmController` contract where the amount of reward given is determined by a preset rate and the length of time that has elapsed.
```solidity=26
    function calculateReward(uint256 start, uint256 end)
        public
        view
        returns (uint256 reward)
    {
        uint256 len = timeRate.length;
        reward = 0;
        for (uint256 i = 0; i < len; ++i) {
            if (i + 1 < len && start >= timeRate[i + 1].startTime) continue;
            uint256 l = timeRate[i].startTime;
            if (end <= l) break;
            if (l < start) l = start;
            uint256 r;
            if (i + 1 < len) {
                r = timeRate[i + 1].startTime;
            }
            if (r > end) r = end;
            reward = reward + timeRate[i].rate * (r - l);
        }
    }
```
However, if `end` is greater than `timeRate[len - 1].startTime`, the function reverts since in the `for` loop when index `i = len - 1`, we will have `r = 0`, causing the update to reward to revert as `r - l` will have an underflow error.

This means that users will be unable to withdraw in the `FarmController` contract after `timeRate[len - 1].startTime`.

Possible Revert When Calculating Rewards

The function `initialize()` sets important variables such as the rates that determine the amount of rewards minted in the `FarmController` contract. As such, there should be access control to ensure that an attacker does not initialize the contract.

Initialization of `PPIRate` Lacks Restrictions

In the contract `PPIToken` the role `_owner` has authority over the functions shown in the diagram below.

![](https://accelerator-tasks-prod.s3.amazonaws.com/9fe95d154c6d415193b9f84c466a9c72/diagrams/centralization-PPIToken-_owner.svg)

In addition, the `_owner` has authority over the functions
- `Ownable.renounceOwnership()`, which disables all `_owner` privileged functions;
- `Ownable.transferOwnership()`, which transfers the `_owner` role to an address.

Any compromise to the `_owner` account may allow the hacker to take advantage of this authority and mint tokens to themselves.

Centralization Risk in `PPIToken.sol`

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.

Susceptible to Signature Malleability

In the contract `SwappiFactory` the role `feeToSetter` has authority over the functions shown in the diagram below.

![](https://accelerator-tasks-prod.s3.amazonaws.com/9fe95d154c6d415193b9f84c466a9c72/diagrams/centralization-SwappiFactory-feeToSetter.svg)

The privileged functions have the following effects:
- `SwappiFactory.setFeeTo()` decides the address that receives fees from minting and burning liquidity tokens;
- `SwappiFactory.setFeeToSetter()` decides the address of `feeToSetter`.

Any compromise to the `feeToSetter` account may allow the hacker to take advantage of this authority and steal fees.

Centralization Risk in `SwappiFactory.sol`

The linked statement does not affect the functionality of the codebase as the `pairFor()` function is a `pure` function.

Redundant Statement

In the contract `SwappiPair` the role `factory` has authority over the functions shown in the diagram below.

![](https://accelerator-tasks-prod.s3.amazonaws.com/9fe95d154c6d415193b9f84c466a9c72/diagrams/centralization-SwappiPair-factory.svg)

Any compromise to the `factory` account may allow the hacker to take advantage of this authority and decide the tokens used for the liquidity pool.

Centralization Risk in `SwappiPair.sol`

When users add or remove LP tokens into the router, the `mint` and `burn` operations are performed. When transferring standard ERC20 deflationary tokens, the input amount may not be equal to the received amount due to the charged transaction fee. As a result, an inconsistency with amount balances will occur and the transaction may fail due to the validation checks.

Incompatibility With Deflationary Tokens

The function `getAmountIn()` is used to calculate the amount of asset needed to receive a given output amount.

```solidity=269
    function getAmountIn(uint amountOut, uint reserveIn, uint reserveOut) public pure override returns (uint amountIn) {
        return SwappiLibrary.getAmountOut(amountOut, reserveIn, reserveOut);
    }
```

However, the function calls `SwappiLibrary.getAmountOut()`, which calculates the output amount for a given input amount.

`getAmountIn()` Calls the Incorrect Function

In the contract `UpgradeableBeacon` the role `_owner` has authority over the functions shown in the diagram below.

![](https://accelerator-tasks-prod.s3.amazonaws.com/9fe95d154c6d415193b9f84c466a9c72/diagrams/centralization-UpgradeableBeacon-_owner.svg)

Any compromise to the `_owner` account may allow the hacker to take advantage of this authority and decide the implementation of the beacon, possibly to a malicious contract.

Centralization Risk in `UpgradeableBeacon.sol`

In the contract `VotingEscrow` the role `_whitelistAdmins` has authority over the following functions:
- `WhitelistedRole.addWhitelisted()`, which makes an address whitelisted;
- `WhitelistedRole.removeWhitelisted()`, which removes a whitelisted address;
- `WhitelistAdminRole.addWhitelistAdmin()`, which gives an account the `_whitelistAdmins` role.

The purpose of the whitelist is to decide which contracts are allowed to create locks. Hence a compromise to a `_whitelistAdmins` account may allow the hacker to take advantage of this authority and prevent a contract users rely on to create locks from creating locks.

Centralization Risk in `VotingEscrow.sol`

The function `_checkpoint()` takes four parameters that are used to update the current supply.

```solidity=251
    function _checkpoint(
        uint256 _newAmount,
        uint256 _newUnlockTime,
        uint256 _oldAmount,
        uint256 _oldUnlockTime
    )
```

where `_oldAmount` and `_oldUnlockTime` are used to decrease the future supply while `_newAmount` and `_newUnlockTime` are used to increase the future supply.

The function `increaseAmount()` invokes `_checkpoint()` with the following arguments:
```solidity=120
        _checkpoint(user.amount, user.unlockTime, newAmount, user.unlockTime);
```
where `user.amount` is the user's old locked amount and `newAmount` is the user's new locked amount. As these arguments are placed in the wrong position, the total supply will be less than it should be.

In addition, the function `increaseUnlockTime()` makes the following invocation of `_checkpoint()`:
```solidity=149
        _checkpoint(user.amount, user.unlockTime, user.amount, _unlockTime);
```
where `user.unlockTime` is the user's old locked time and `_unlockTime` is the user's new locked time. Similarly, these arguments should be switched or else the total supply will be incorrect.

Incorrect Arguments Used When Creating a Checkpoint

The function `initialize()` sets important variables such as the voting escrow token information, the token allowed for creating locks, and the maximum time a lock can be created for. As such, there should be access control to ensure that an attacker does not initialize the contract.

Initialization of `VotingEscrow` Lacks Restrictions

The supply is updated through `_checkpoint()`, which is called whenever a lock is created or updated. If there is a long period of time where `_checkpoint()` is not called, then the next user to create or update a lock may have to pay for an expensive transaction, which will deter users from interacting with the contract.

Swappi Finance

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

44

0

18

0

26