Audit done by Saksham
https://github.com/pandai-corp/pandai-earn-sc/tree/main/contracts
PandAI.sol
USDT.sol
PandAIEarn.sol
To get the old lpAddress the code here https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L173
assigns oldLpAddress = newLpAddress , this is wrong the oldLpAddress should be lpAddress instead , because of this
emit LpAddressChanged(oldLpAddress, newLpAddress) this event would emit same values for both oldLpAddress and newLpAddress
i.e newLpAddress.
Eventhough the comments mentions the risk that rewards may be lost if claim is not called prior to calling depositWithReferral and requestWithdraw , it should be a mandate or in other words call claim() within these function at the beginning so that users never experience this loss in rewards. A user might not read carefully/neglect the guidlines/comments and hence lose rewards.
As the tier increases , so does the minimum deposit for the tier(see constructor for reference) , and the tier is calculated according to the total deposits made by the user (https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L387) , in short more the deposit by a user higher would be the tier of the user. But here https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L216 , to check if the deposited USDT is above the minimum we are always checking against the first tier value instead of comparing it to the correct tier. An attacker can keep depositing above the first tier minDeposit even though he belongs to a higher tier.
Correct way would be to do it like this ->
uint8 tier = getUserTier(msg.sender);
require(usdtDepositAmount >= tierMap[tier].minDeposit * (10 ** usdtToken.decimals()), "small deposit");
Part-2(requestWithdraw function) - MinDeposit of tier 1 is checked always instead of checking for the respective tier
As the tier increases , so does the minimum deposit for the tier(see constructor for reference) , and the tier is calculated according to the total deposits made by the user (https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L387) , in short more the deposit by a user higher would be the tier of the user. But here https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L250 we check if the remaining deposit for the user is at least the minimum deposit , but we only check against the min deposit of the first tier , it is possible that the user belongs to a higher tier , and hence the possibility arises where less deposit would be left than what it should be.
Same remediation as above.
We need to ensure that after a withdrawal , the user deposit left is at least the minimum deposit , we check that here
https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L250 .
Now let's consider the case where usdtWithdrawAmount = userMap[msg.sender].deposit
(1.) This condition would pass this check here https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L248
(2.) This condition would skip the condition here https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L249
Because of above 2 point , when the code reaches here https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L267
, userMap[msg.sender].deposit would be 0 , and this violates the minimum deposit rule.
Change if (usdtWithdrawAmount < userMap[msg.sender].deposit) to if (usdtWithdrawAmount <= userMap[msg.sender].deposit)
The function getPandaiWorthOf is used to calculate the Pandai worth per USDT , and is used in functions to calculate
fee , i.e here https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L419 and here https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L437 . So in a price manipulation attack as follows -
a. Attacker gets a arbitrage/price manipulation oppurtunity from a swap and deposits large amount of USDT into the LP.
b. According to https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L381
the price that would be calculated would be highly deflated as USDT is much more than Pandai in the LP.
c. Attacker's fees calculated here https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L423 would be
much less than what it was supposed to be due to price deflation.
Use a decentralised oracle like chainlink to get the price Of Pandai/USDT
From the name of the function getNewReferralReward gives an amount that would be paid to the referral , when a user with
that referral as referrer makes a deposit here https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L215
, so it should be only invoked when a user makes a deposit.
There is no need to call this function here https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L272
and here https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L313 , invoking the functions here would
just unnecessarily increase the pending reward of the referral .
Consider calling the getNewReferralReward only when a user calls depositWithReferral
userMap[msg.sender].deposit increases when a user deposits and so does the userMap[userMap[msg.sender].referral].referralDeposit
with the same value (See line 226 and 232). referralDeposit increases when a user using that referral deposits , this means
referral deposit would at least be equal to userMap[msg.sender].deposit (considering only msg.sender made the deposit using the referral).
Here https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L248 we check if user's deposit is at least what the user is trying to withdraw , this condition should also automatically imply referral deposit is also atleast the withdraw amount (according to the above paragraph).
Therefore the condition here https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L273 would be
always true , or in other words no need to check userMap[userMap[msg.sender].referral].referralDeposit >= usdtWithdrawAmount
Deposits are meant to be paused when the contract is paused , but this privileged deposit here https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L189 let's the owner deposit even when the contract is paused.
Owner can at anytime call https://github.com/pandai-corp/pandai-earn-sc/blob/main/contracts/PandAIEarn.sol#L181 and withdraw all the USDT , since the docs/comments mention nowhere that owner is supposed to be a multisig , this is a decentralisation/rugpull issue.
In all the contracts i.e PandAI.sol , USDT.sol , PandAIEarn.sol we have used plain imports insetad of names reports , this would import everything instead of just importing the functionalities we need in the contract.
The current implementation of the claim function is , claim would be called on the user that called the function , to make the claim system more flexible the claim function can be modified such that X person can call the claim for Y user.
The stablecoin's contract has been named USDT while it should have been named something like BUSD cause , USDT has 6 decimals and the implementation which the contract uses has 18 decimals.
Let's take example of the Tier struct , we know the max value for the minDeposit can be 10000 (tier 5) , so this can be made into
uint16 , then minDeposit and compoundInterest can be clubbed together into a single slot (bool takes one bit) , thus saving
20000 gas. Similarly more slots can be optimized.
(1.) Unlocked Pragma Contracts should be deployed using the same compiler version/flags with which they have been tested. Locking the floating pragma, i.e. by not using ^ in pragma solidity ^0.8.7, ensures that contracts do not accidentally get deployed using an older compiler version with unfixed bugs.
(2.) Typos
There are typos throughout the code base and it seems comments were written in a hurry instead of detailed and concise comments.
(3.) Comments Don't Follow Natspec Specification
The comments are written informally , there should be details about every parameter a function uses instead of just mentioning what a function is supposed to do.
(4.) Missing documentation
There was no documentation provided with the code base to better understand the codebase , good documentation helps with understanding the code base better and makes the auditing process faster and easier.