Exchange, Part 1

Privileged addresses have authority over many functions that may be used to negatively disrupt the project. Some important privileges include:

GoldenGate

• owner can withdraw all funds.
• owner can set admins which are able to dilute allocation of other pools.
• owner can set the migrator contract which can lead to loss of LP if malicious.

TokenVesting

• owner can arbitrarily set the fee and fee address which can lead to loss of user funds.

BridgesRef

• feeToSetter can arbitrarily set the distribution rate.
• feeToSetter can withdraw any ERC-20 token in the contract.

BridgesRouter

• feeSetter can set a arbitrary referral and dividend tracker contract.

BridgesFactory

• feeToSetter can set an arbitrary start time for when a pair can be tradeable.

Ensure that the privileged addresses are multi-sig and/or introduce timelock for improved community oversight. Optionally introduce require statements to limit the scope of the exploits that can be carried out by the privileged addresses.

Bridges Team: - All centralized BNB and token withdrawal functions have been removed from the GoldenGate contract,

• The possibility to change fees on the TokenVault has been removed as well.
• The GoldenGate migrator has to be there for an eventual V2 in the future.
• For the same reason all update functions on the tracker are necessary.
• Every privileged address will be a multi-sig with trusted members in production.

Due to the unbounded for loop in distributeDividends, there is a risk of a DoS attack. Anytime a new address deposits to pool 0, they are added to the usersBridges list. A malicious party can keep generating new addresses and deposit miniscule amounts of LP to make distributeDividends exceed the block gas limit, stopping all dividends.

Process the users in smaller batches, set a cap on number of users who can receive dividends, or modify the dividend allocation logic entirely such that a for loop is not needed.

For an alternative approach, see this “pointsPerShare” implementation:

https://github.com/indexed-finance/dividends/tree/master/contracts

Bridges Team: - The dividend distribution mechanism was updated to a dividendsPerShare model.

Because the dividends paid to a user is only updated after the external call sending them funds, it is possible for a malicious contract to re-enter and keep draining div amount of BNB on each call.

Add a nonReentrant modifier from OpenZeppelin’s ReentrancyGuard or utilize the check-effects-interactions pattern.

Bridges Team: - Added the lock modifier to deposit, depositLocked, relock, and withdraw.

In the transferLock function _to.stakeUntil is set to _from.stakeUntil. Therefore, it is possible to shorten the lock period by transferring from an address with a shorter lock to an address with a longer lock.

When transferring a lock, adopt a push then pull pattern where the receiver needs to accept an incoming lock, and then adopt the longer lock period when combining locks. Alternatively, make each lock its own unique NFT token.

Bridges Team: - Now requires that the _to.amount = 0.

Because depositing and withdrawing from pool 0 relies on a successful BNB transfer for the dividends payment, it is possible to prevent deposits and withdrawals. If a user were to drain the BNB from the contract using the re-entracy described earlier or the owner drained the BNB using the BNB function, then the call would fail and the transaction would revert.

Refactor the dividend payments so they are separate from withdrawals and deposits.

Bridges Team: - Dividends have been refactored and the emergency BNB function has been removed.

Because it is possible to publicly see transactions that are sending value to distributeDividends, bots can frontrun the distribution. This way addresses may sandwich a deposit and withdrawal around a distribution in order to unfairly accumulate dividends while never effectively holding the token.

Introduce a warmup period, or require locking for dividends.

Bridges Team: - We think this is unlikely as it would require swapping for tokens and providing/removing liquidity to achieve a return, it would likely be gas/slippage cost prohibitive.

The check user.alreadyHere == false can be simplified to !user.alreadyHere.

Replace user.alreadyHere == false with !user.alreadyHere.

Bridges Team: - The simplification was made.

Withdraw is spelled witdhraw in the error message on line 265.

Correct it to withdraw.

Bridges Team: - Typo has been fixed.

The statement user.rewardDebt = user.amount.mul(pool.accBRGPerShare).div(1e12) is repeated on line 275.

Remove the first occurrence on line 273.

Bridges Team: - The first duplicate was removed.

In the emergencyWithdraw function, the statement user.userLockedAmount = user.userLockedAmount.sub(user.userLockedAmount) is equivalent to user.userLockedAmount = 0.

Replace the inefficient statement with user.userLockedAmount = 0.

Bridges Team: - Updated to = 0.

In the Bep20 and BNB functions the require statements specify a value < the contract balance, meanwhile it may be intended to remove a value equal to the contract balance.

Confirm whether or not the exact contract balance should be able to be withdrawn and optionally update accordingly.

Bridges Team: - Updated to <=

In the Bep20 function the payable cast on line 331 is unnecessary as the safeTransfer function simply accepts an address.

Remove the payable cast.

Bridges Team: - Removed the payable cast.

The function pendingbridges does not abide by camelCase naming conventions.

Rename it to pendingBridges.

Bridges Team: - The function has been renamed.

The shares variable is initialized to 0. This is unnecessary because the default value for the uint256 type is 0.

Remove the assignment.

Bridges Team: - Removed the assignment.

Because the trading fee tradingFee is taken before amounts is calculated, there may not be enough BNB to deposit into the WBNB contract. Thus, the transaction will revert and the swap will fail.

Take the trading fee after the swap has occurred, or account for the trading fee in getAmountsIn.

Bridges Team: - Added amounts adjustment for the fee.

In the distribute function each user.earned amount is multiplied by the disRate but in the withdraw function, the user.earned amount is not divided by some divisor that corresponds to the disRate.

Add a divisor for the disRate and use it to adjust user.earned values in either the distribute or withdraw functions as needed.

Bridges Team: - This is intended behavior, we treat the disRate as a multiplier.

There is no bound to how high the disRate variable can be set in the setDisRate function. Therefore, the distributed amount in the distribute function may exceed the provided amount.

Implement a maximum cap for disRate. A cap of ~ 57% (1/1.75) would allow for a maximum distribution of amount.

Bridges Team: - Set a disRate cap of 1000.

The Distributed variable is not referenced at all.

Remove the Distributed variable.

Bridges Team: - The variable has been removed.

The withelistTokens, withelistUsers, and withelistUser functions all have a typo.

Correct them to whitelistTokens, whitelistUsers, and whitelistUser.

Bridges Team: - Typos have been corrected.

In the withdraw function the require statement specifies a value < the contract balance, meanwhile it may be intended to remove a value equal to the contract balance.

Confirm whether or not the exact contract balance should be able to be withdrawn and optionally update accordingly.

Bridges Team: - Updated to <= to be able to withdraw the max amount.

The BridgesFactory contract fails to provide implementations for the feeTo, tradingStart, and setFeeTo functions defined in the IBridgesFactory interface.

Either add implementations for these functions in the BridgesFactory contract or remove them from the IBridgesFactory interface.

Bridges Team: - Unimplemented functions have been removed from the interface.

In the mint function, the only precondition for adding an address to the users list is if the balance of the address is 0. Additionally, an address is not removed from the users list if it transfers it’s balance of the BridgesPair token.

This way an address can continually mint and transfer/burn it’s tokens to enter the users list multiple times. Multiple entries in the users list will result in multiple dividends being paid out in the distributeDividends function.

Add a check for addresses that are already in the users list.

Bridges Team: - Dividends have been refactored to a dividendsPerShare implementation.

Due to the unbounded for loop in distributeDividends, there is a risk of a DoS attack. A malicious party can keep generating new addresses and minting minimal amounts of the BridgesPair token to make distributeDividends exceed the block gas limit, stopping all dividends.

Process the users in smaller batches, set a cap on number of users who can receive dividends, or modify the dividend allocation logic entirely such that a for loop is not needed.

For an alternative approach, see this “pointsPerShare” implementation:

https://github.com/indexed-finance/dividends/tree/master/contracts

Bridges Team: - Dividends have been refactored to a dividendsPerShare implementation.