unifarm - Bridge

QuillAudits8 Aug 2022Code version: not recorded (see the report)

Audit · firm’s term: audit report

QuillAudits reviewed UniFarm (unifarm - Bridge) on 8 Aug 2022: 34 findings, 0 critical; 23 marked fixed, 11 open.

No open critical findings

Open the report Open UniFarm’s security record

Report hosted on quillaudits.com.

Trustblock does not review code.

Findings34

Statuses are the security firm’s own, from its report.

Severity	Open	Fixed
Critical	0	0
High	0	1
Medium	3	10
Low	8	12

HighReetrancy attack in removeLiquidity()Fixed

There is possibility of a profitable reentrancy attack in removeLiquidity(). This is because a low level call is used in transferLPFee() function which allows the caller to call any malicious code from another contract. Using this multiple rewards for same liquidity position can be harvested.For example, Let’s say the 1st liquidity position by an LP is 1000 USDC. After that a user does cross-chain transfer. Now the LP sees that he has some rewards available to claim. Then he adds a 2nd position of say 5000 USDC tokens. Now when the user does a reentrant call via low level call in transferLpFee, he will be able to get twice the reward for the same liquidity position. This is possible because although the index of the first liquidity position will be used for the reentrant call, 2000 or more USDC tokens will need to be withdrawn for claiming more rewards which will be taken from the LP’s second liquidity position directly. The rest of the liquidity he will still be able to claim via transferOut.

MediumCentralization of addTokenMetadata()Fixed

A malicious owner can deploy a dummy or fake setu token and overwrite the existing metadata of already added setu token using addTokenMetadata using addTokenMetadata() function. For example, let's say setuUSDC was deployed using addBridge() for USDC token, then a malicious owner can overwrite the bridgeTokenMetadata mapping for the USDC setuToken. This is possible if the owner had deployed an ERC20 token with symbol as setuUSDC and the address of this token is passed as a parameter to the addTokenMetadata() function. This attack can be coupled with A.1 to amplify this attack. In addition to this, overwriting to bridgeTokenMetadata can also be done via _deploySetuToken if a setuToken with the same ticker and symbol is deployed again by an owner.

MediumCentralization of burnTokens() allows burning tokens from any accountFixed

burnTokens() function can be called by the owner to burn tokens from any account, even from the accounts that it currently does not own. Here the owner is a bridge contract, so this intended design is fine. But this can be exploited by a malicious admin if he calls renounceOwnership() and transfer ownership of the contract to himself. Also lockedTokens() function can be called by the owner to burn tokens from any account, even from the accounts that it currently does not own. Also a malicious owner can mint more setuTokens and use it to drain liquidity from the Liquidity Pools by calling the transferOut() function.

MediumCentralization of safeWithdrawLiquidity()Fixed

safeWithdrawLiquidity() can be used by a malicious owner to withdraw all the liquidity of any token.

MediumCentralization of updateFeeConfig() and updateEpochLength()Fixed

updateFeeConfig() allows updating of tokenBridge's fee at any time. Also the _feeInBips parameter has no upper limit and can be exploited by a malicious admin to set very high fees without anyone noticing or taking action. Also _feeInBips parameter lacks zero value check as well which can lead to unintended issues such as no fees getting paid by users. updateEpochLength() allows to update the epochLength of bridge anytime which can lead to unintended issues and bugs.

MediumChildToken cannot be upgraded by ownerFixed

Although the childToken is an upgradeableToken, it can never be upgraded by the owner of the Registry Contract. It can only be upgradedby the proxyAdmin. The Registry contract is the owner of the proxyAdmin, not the owner of the Registry contract.

MediumLoss of BoostersFixed

If a user buys booster packs using buyBoosterPacks() and then add liquidity using addLiquidity(), then the user will lose his currently bought boosters due to overwriting of boosterEndEpochIndex for that particular liquidity position. For example, let's say user buys 1000 booster packs for token USDC, with _index parameter specified as 1 (for which he has not added any liquidity yet and let's say currentEpochIndex is 50). This will set boosterEndEpochIndex to 1049. Now at epoch say 60, and the user adds liquidity using addLiquidity() with _noOfBoosters as zero, the boosterEndEpochIndex will get overwritten to 60 resulting in loss of boosters bought.

MediumMissing usage of pausable modifierFixed

Pausable modifier from the Openzeppelin is not used. Calling pauseTokens() does not actually pause any functionality in the contract like locktokens() or unlocktokens().

MediumNo sanity checks for bridgeType in addBridge()Fixed

There are no sanity checks for bridgeType in addBridge() function. According to the docs provided there can be a maximum of 2 bridge types- liquidity bridge and child bridge (or liquidity + child bridge). BridgeUpgradeable contract's transferIn() which has if statements depending on bridgeType, will not get executed if the bridgeType is incorrectly set other than value 0 or 1. Also this could lead to the possibility that the crossChainTransferIn() is done successfully without paying any fees (plus the user gets additional setutokens minted which would actually should have been burned).

MediumPossibility of revert after _deploySetuToken()Fixed

If _deploySetuToken is called externally without adding any bridge via addBridge(), the function disableBridge() will revert always for the setuToken's primary token. Also updateFeeConfig() and updateEpochLength() will not work as it will revert due to the first require check.

Show 10 more

MediumremoveBridge() can lead to Denial of ServiceFixed

If an existing bridge is removed using removeBridge() which contains liquidity provided by LPs, they will not be able to claim back their liquidity. This would also result in LPs unable to claim any fees accumulated till that point as well.

Low_gap variable not requiredFixed

__gap variable is not required in BridgeUpgradeable contract (declared at the end of the contract) because __gap variables are generally used in Base contracts and not in Child contracts. It may lead to unintended issues or mistakes.

LowMissing require check for feeTypeFixed

According to the docs feeType can only be 0 or 1. But updateFeeConfig() allows to set fee for any fee type.

LowMissing zero address checkFixed

Missing zero address check for _newBridgeAddress parameter in the updateBridgeAddress() function. This could lead to transfer of ownership of newChildToken to zero address if deployChildToken() function is called. Similarly if addBridge() function is called, _deploySetuToken() function will be called internally, leading to transfer of ownership of setuToken to zero address.

LowMissing zero address checkFixed

Missing zero address check in for _admin parameter in initialize() function.

LowNaming conventions not followed thoroughlyFixed

Solidity naming conventions are not followed thoroughly. For example: Internal function should be named _addPassedEpochs() and external function should be named addPassedEpochs(). Also as _deploySetuToken() is a public function, it should be named deploySetuToken() instead.

LowPossibility of reentrancyFixed

The functions addLiquidity(), crossChainTransferIn(), crossChainTransferOut(), transferIn() can be vulnerable to reentrancy attack if setutoken is malicious.

LowRedundant codeFixed

The if statement checks whether the mapping is zero and then deletes the mapping. But this is not needed because delete does the same thing as setting the mapping to zero value.

LowReentrancy in transferOut()Fixed

A reentrant attack is possible via transferOut() function because of the low level call used in calculateBridgingFees() function. The low level call allows another account to call malicious code although in this case it would just be a griefing attack as a simple reentrant call would only result in the caller paying more fees.But there is still the possibility of a cross-contract reentrancy attack if the contracts in question used shared variables which can not even be mitigated by using non-Reentrant modifier from Openzeppelin. Note that the function crossChainTransferIn() will also be vulnerable to the same attack as transferOut is being called internally there.

LowRenounce ownershipFixed

The renounceOwnership function can be called accidentally by the admin leading to immediate renouncement of ownership to zero address after whichany onlyOwner functions will not be callable which can be risky.

Show 3 more

LowTransfer ownershipFixed

The transferOwnership() function in contract allows the current adminto transfer his privileges to another address. However, inside transferOwnership() , the newOwner is directly stored into the storage owner, after validating the newOwner is a non-zero address, and immediately overwrites the current owner. This can lead to cases where the admin has transferred ownership to an incorrect address and wants to revoke the transfer of ownership or in the cases where the current admin comes to know that the new admin has lost access to his account.

LowUnused internal functionFixed

getLPsFee() is an internal function that is not getting called anywhere.

LowupdateBoosterConfig() lacks sanity checksFixed

The updateBoosterConfig() function lacks sanity checks for its parameters such as _perBoosterPrice and _adminAccount and thus can be accidentally set to incorrect values.