search

Byte-Masons / Reliquary

50 stars 20 forks source link

Audit Review from snapshot commit c638a8bfb2842bd33d16093641b4635f07b8634e #9

Closed StErMi closed 1 year ago

StErMi commented 2 years ago

Disclaimer

This document does not constitute legal or investment advice.

The author is not responsible for any errors or omissions, or for the results obtained from the use of this information. All information in this document is provided "as is", with no guarantee of completeness, accuracy, timeliness, or of the results obtained from the use of this information.

Note

As I said in the title this review has been made while the code is changing so bear with me if something is already different. In order to go more in-depth with some logic/math, I would love to have a proper overview of the project.

Review

This review has been made on code from master branch. The snapshot has been taken on commit hash c638a8bfb2842bd33d16093641b4635f07b8634e

Notes

I would have some more in-depth documentation on some mechanisms (these are maybe something that people that work in the DeFi space already know and are taken as basic assumptions).

Pragma lock

At the moments all the contracts have unlocked pragma like this pragma solidity ^0.8.0; For security reason it should be better to choose a compiler version and lock all your contract to a specific version (https://swcregistry.io/docs/SWC-103)

Refactors

1) Don't know if there are some gas overhead (transmission11 said that it would have some gas overhead calling internal functions but need to check) but you could replace all lpToken[_pid].balanceOf(address(this)); to _totalDeposits(_pid) 2) all the maths to calc oaths/and so on should have a separate function for better readability 3) curved function could be renamed calculatePositionMean and _calculateMean could be renamed _calculatePoolMean? because curved and _calculateMean do the same thing with the difference that the first one does it for the user's position entry and the second for the whole LP average position entry 4) _calculateDistanceFromMean instead of using memory (expensive) could just returns (uint256, Placement) 5) generally speaking following the Checks-Effects-Interactions Pattern you should always do any state modification and event emission before external calls that could mutate your contract's state logic. In some cases you can't do that because those external calls are needed to change the state but at least at some point, you could do that like event emitting (see withdrawAndHarvest). I know that it feels an "anti-pattern" and not natural because you would emit an event only at the end of the function's logic but it's still a good practice to follow in Solidity.

Issue 1: migrate all the comments in a natspec format

The current code documentation (where available) is not respecting the natspec format. It would be useful for dev/auditors but also for users (etherscan now support natspec for verified contracts) to have the code commented in natspec.

Issue 2: check _oath in construct

OATH is an immutable variable. Double-check that it's != address(0) because after setting it in the constructor there's no way to update it

Issue 3: change from public to external where possible for gas optimization

functions like

should be changed from public to external for gas optimization.

Issue 4: Move from "simple" require to custom errors

Whenever you revert with a string message this will consume at least (it depends on the length of the string) 256 bits. Custom errors are more UX-friendly and could consume less gas. Anyway, given that we are on FTM, even if the gas is not a problem, they still give a better UX where you can specify for example which were the parameter that has failed and why.

Issue 5: Add params check on add

Check that user parameters like _lpToken, _rewarder and _curve are != address(0)

Issue 6: Possible refactor of set

Given that both the pool's rewarder and curve should not be address(0) you could remove both overwriteRewarder and overwriteCurve and simply allow those parameters to be address(0) if you don't want to change them.

    function set(
        uint256 _pid,
        uint256 _allocPoint,
        IRewarder _rewarder,
        address _curve
    ) public onlyOwner {
        require(_pid < poolInfo.length, "set: pool does not exist");
        totalAllocPoint = (totalAllocPoint - poolInfo[_pid].allocPoint) + _allocPoint;
        poolInfo[_pid].allocPoint = _allocPoint;
        if (address(_rewarder) != address(0)) {
            rewarder[_pid] = _rewarder;
        }
        if (_curve != address(0)) {
            poolInfo[_pid].curveAddress = _curve;
        }
        emit LogSetPool(
            _pid,
            _allocPoint,
            address(_rewarder) != address(0) ? _rewarder : rewarder[_pid],
            _curve != address(0) ? _curve : poolInfo[_pid].curveAddress
        );
    }

Issue 7: missmach between add and set function signature

while in add the _curve parameter is expressed as a ICurve on set is declared as an address. Update the set parameter to use the same param signature as add

Issue 8: consideration about updatePool

No check on pid existance

This is not a problem per se because if that pool does not exist pool.lastRewardTime would be 0 (default value) so it won't enter the if statement.

But the off-chain/onchain service that has called that method would not know that it has failed. The only way to enforce that is to force the caller to check the returned result and see if it's not initialized.

Personally, I would revert the tx in order to let know the caller that it has tried to update a not existing pool

should the pool update the lastRewardTime and emit event if the Reliquary lp balance is 0?

If the lp balance of the Reliquary is 0 it correctly avoids updating the accrued oaths (I think that it would anyway revert with a panic error because of division by 0) but it still updates the lastRewardTime and emits the event.

This could be a perfectly fine behavior because it tracks it even if there's no real change (if not the lastRewardTime). Just double-check that this is the intended behavior.

Issue 9: Mixed usage of code style for function's returns

I see that sometimes you use names of return variables and sometimes not. Personally, I would prefer to avoid it in order to explicitly return when needed the parameter.

Anyway, just to be coherent in code style adopt just one style and update the rest of the code to not create confusion in other devs/auditors.

Issue 10: createNewPosition questions

1) I think that the nonReentrant guard here is unnecessary. mint function should not need to be nonReentrant per se. it should theoretically be safe 2) I would add a nonReentrant on the deposit that will also automatically protect the upper call of createPositionAndDeposit (still need to check if it really needs nonReentrant but it's not so much gas wasted) 3) I do not understand the reason to allow people to create positions without depositing into them. Could you please explain better the meaning of this function called alone? I understand if called by createPositionAndDeposit but not alone 4) can someone mint a Reliquary NFT without being associated with a pool? Is it correct? 5) why could someone open a position for someone else?

Issue 11: deposit problems/questions

1) amount > 0 should be amount != 0 for gas optimisation 2) why isn't there a check on pid existence? can someone deposit on a pool that does not exist? 3) could you explain why a pool could have an empty rewarder (address(0)) 4) lpToken could be an exploit vector if it does not follow the ERC20 standard and has custom code. Be double sure that you whitelist which tokens are used as LP for the Pool or just add a reentrancy guard on deposit 5) Is there a reason why you check the balance before and after the transfer on lpToken?

uint256 _before = lpToken[pid].balanceOf(address(this));
lpToken[pid].safeTransferFrom(msg.sender, address(this), amount);
uint256 _after = lpToken[pid].balanceOf(address(this)) - _before;

Maybe I'm lacking some knowledge but if lpToken is a standard token without any tax or deflationary/inflationary mechanism that transfer should transfer the amount without changing it. I know that we're on FTM and at the moment gas is cheap but if this is not something important you could just assume that the whole amount has been correctly sent to the lpToken and you can avoid 2 external calls (remember that .balanceOf are still external calls that cost gas because they read state)

6) _after name is misleading. _before is the amount of balance of the LP before the user has transferred the deposit. _after is instead the balanceAfterTransfer-_before so it should be called transferredAmount. It took me some time to understand why you were using the whole LP balance in the rewardDebt calc thinking that _after was the balance after transfer because _before was the balance before transfer. 7) if you are unsure about the amount (because of those checks you do before/after transfer) maybe you should use _after also on the Deposit event instead of using amount directly because that event parameter should be the real amount deposited in the LP

Issue 12: _updateAverageEntry problems

wrong assumptions about LP token decimals

It's true that each ERC20 contract made with OZ has 18 decimals but people can simply override it and still be an ERC20 token. ERC20 does not enforce tokens to have 18 decimals.

In functions like _updateAverageEntry you are making a big assumption because you are multiplying and dividing by 1e18

Be SUPER sure that those tokens that you use as LP Tokens are all with 18 decimals.

Issue 13: withdraw problems/questions

1) amount > 0 should be amount != 0 for gas optimisation 2) why isn't there a check on pid existence? can someone deposit on a pool that does not exist? 3) the second call to address to = ownerOf(positionId); is useless, you have already checked that the msg.sender is the owner with the first require so to would always be msg.sender 4) could you explain why a pool could have an empty rewarder (address(0)) 5) why don't you do the same before/after check to double-check the amount that has been really withdrawn from the pool?

Issue 14: _modifyEmissions has a misleading name

_modifyEmissions is a view function that does not modify the state. Change the name reflecting the purpose the of the function

Issue 15: harvest problems/questions

1) could you explain why a pool could have an empty rewarder (address(0))

Issue 16: withdrawAndHarvest problems/questions

1) amount > 0 should be amount != 0 for gas optimisation 2) OATH.safeTransfer(to, _curvedOath); should check if the _curvedOath is != 0 like you do in harvest 3) the second call to address to = ownerOf(positionId); is useless, you have already checked that the msg.sender is the owner with the first require so to would always be msg.sender 4) could you explain why a pool could have an empty rewarder (address(0))

Issue 17: emergencyWithdraw problems/questions

1) second call to address to = ownerOf(positionId); is useless, you have already checked 2) should the function revert if amount is 0? If the amount is zero can the user have pending rewards? I would anyway revert in order to not allow people to spam EmergencyWithdraw that could screw up monitoring tools like Defender or create FUD for people that are monitoring this contract 3) Should the withdrawal also reset the pending rewards? Isn't a way to allow both forces to withdraw without resetting the rewards and allow them to pull the rewards in a second moment? Anyway because the emergencyWithdraw is an emergency call it should be the last of your problem to not harvest reward at this point but I need more explanation

Issue 18: onOathReward unknown behavior

At the moment I've zero knowledge on how it should work because the RewarderMock has only commented out code. But if the Rewarder follow the commented code you should avoid any call to oathAmount where oathAmount == 0 because it's a waste of gas (it will try to send to the user 0 tokens)

Could you please explain more about this contract and how it should interact with the OATH?

Issue 19: Oath can be minted by anyone. Is it normal? Shouldn't be mintable only by Reliquary or anyway not public mintable?

Zokunei commented 2 years ago

Notes

These are all concepts borrowed from Sushi's original MasterChefV2 contract we forked from, as it is the basis for most LP farms in DeFi.

  1. Implemented in commit ab15555903b8140b1f6de8625b3079dd241bec0c, seems to actually save gas
  2. Likely to implement
  3. Although the functions may look similar, curved doesn't calculate a mean value, so another name (possibly just the current one) may fit better
  4. Implemented in commit 02704b0137196597f770d681b8e8884121225ccc
  5. Will consider, but low priority since ReentrancyGuard covers this function and emit does not modify state

    Issue 1: migrate all the comments in a natspec format

    Will implement

    Issue 2: check _oath in constructor

    Will consider, but may be unnecessary modification of MasterChefV2

    Issue 3: change from public to external where possible for gas optimization

    Will consider, but may be unnecessary modification of MasterChefV2

    Issue 4: Move from "simple" require to custom errors

    Will consider, but unsure if there is a standard practice for implementing this

    Issue 5: Add params check on add

    Will consider, but may be unnecessary modification of MasterChefV2

    Issue 6: Possible refactor of set

    Will consider, but may be unnecessary modification of MasterChefV2

    Issue 7: missmach between add and set function signature

    Implemented in commit af2d0eab830b46cb3781f2f7b8c74e3921c9e7f1

    Issue 8: consideration about updatePool

    Will consider, but may be unnecessary modification of MasterChefV2

    Issue 9: Mixed usage of code style for function's returns

    Implemented in commit 44cd91266da01a2d7595a261d152f178204c0c98

    ~Issue 10: createNewPosition questions~

Removed createNewPosition function in commit 0b1adc31697a15d3395e472ab856cb77676c56e5

Issue 11: deposit problems/questions

  1. Implemented in commit a448c21b6eee54bc7988a106ff8ee036600ab6f5

  2. No, it fails with array index out of bounds as in original MasterChefV2

  3. Most farms do not have a rewarder since rewards are already given by the MasterChef (or Reliquary in this case)

  4. ReentrancyGuard added in commit 35c4153122e4323d07f07dbf0fa14c82cc4fdcc5 (deposit calls updatePool)

  5. This is done in order to support e.g. burn-on-transfer tokens

  6. Implemented in commit f1838264e0f797a68de03f23e5300b864b4d8e29

  7. Implemented in commit f1838264e0f797a68de03f23e5300b864b4d8e29

    Issue 12: _updateAverageEntry problems

    It seems to me this isn't a security issue in this context, but the current implementation rather avoids an unnecessary external call to lpToken[pid].decimals(). The amount weight affects averageEntry will still reflect the proper proportion between amount and lpSupply since multiplying and dividing by 1e18 will only use more precision than necessary. We are not calculating a balance or amount of tokens to transfer here. I would like to hear from others on this, but I don't think this would actuallly cause any problem using a token with less than 18 decimals.

    Issue 13: withdraw problems/questions

  8. Implemented in commit a448c21b6eee54bc7988a106ff8ee036600ab6f5

  9. No, it fails with array index out of bounds as in original MasterChefV2

  10. Implemented in commit 740afd57cc78e9a313620c3f73738b9ce9f7536f

  11. Most farms do not have a rewarder since rewards are already given by the MasterChef (or Reliquary in this case)

  12. Implemented in commit ea65482fb6dc95cb06b2c3d94833c49f3306a68f

    Issue 14: _modifyEmissions has a misleading name

    Implemented in commit 6fbba710a11b50114de90ad480eb3ad2be8c815f

    Issue 15: harvest problems/questions

    Most farms do not have a rewarder since rewards are already given by the MasterChef (or Reliquary in this case)

    Issue 16: withdrawAndHarvest problems/questions

  13. Implemented in commit a448c21b6eee54bc7988a106ff8ee036600ab6f5

  14. Implemented in commit 647c24b27563ca5d9e278f5eae4ab266725c7e3d

  15. Implemented in commit 647c24b27563ca5d9e278f5eae4ab266725c7e3d

  16. Most farms do not have a rewarder since rewards are already given by the MasterChef (or Reliquary in this case)

    Issue 17: emergencyWithdraw problems/questions

  17. Implemented in commit eac86aff8a3817e87df1a88407891f2ba4921aa0

  18. Will consider, but may be unnecessary modification of MasterChefV2

  19. Will consider, but may be unnecessary modification of MasterChefV2

    Issue 18: onOathReward unknown behavior

    See RewarderMock.sol Will restore functionality to our RewarderMock and keep support for rewarders

    Issue 19: Oath can be minted by anyone. Is it normal? Shouldn't be mintable only by Reliquary or anyway not public mintable?

    This is for testing purposes only. We need to implement a real Oath contract.

0xBebis commented 2 years ago

Issue 1: agreed, tried to follow natspec format but we can hit it harder Issue 2: Maybe someone making this mistake shouldn't be using the software? But in all seriousness I think it's unnecessary. It's likelier that someone would use the WRONG address than the zero address, which we couldn't account for anyway. The real solution here is to make our deployment script really easy to configure and difficult to fuck up. Issue 3: This may effect extensibility for little benefit. Issue 4: The real consideration here I think is making all our error messages constants so we don't have to compile them. Maybe some gas/code size savings to be explored here. Also standardizing error message formats to : could be wise. Let me know what you think. issue 5: See my answer to issue 2. issue 6: I can see the benefit, but I think passing a 0 value into the address to skip it is confusing UX. We can handle a lot of this with how we write all the scripts we package with it. Issue 7: Good call, I see Zokunei already took care of this Issue 8: I think we can leave this as-is Issue 9: Good callout Issue 10: We discussed this and removed Issue 11: I agree with Zokunei's assessment of the issues Issue 12: I think we can make a note to double check the system's ability to handle non-standard decimals. an internal _decimals() function with input/output control and doing 10 ** _decimals() whenever we need to normalize operations. Just making it dummy proof for people who fork the code. Issue 13: Agree with Zokunei's assessments Issue 14: I dig the change Issue 15: see Zokunei's comments Issue 16: I agree with these changes Issue 17:

  1. good callout
  2. good theory, bad practice. This is mostly an enterprise function.
  3. The goal of emergency functions like this is to bypass as much logic as possible to increase the chances of successful execution. 18: See Zokunei's comments 19: See Zokunei's comments

Thank you Stermi! Excellent review.

StErMi commented 2 years ago

Issue 1: agreed, tried to follow natspec format but we can hit it harder It harder

No natspec are different both on how you declare the comments /// or /** blabla */ and how you declare @notice, @dev, @param, @return and so on. More info here: https://docs.soliditylang.org/en/v0.8.11/natspec-format.html

Issue 2: Maybe someone making this mistake shouldn't be using the software? But in all seriousness I think it's unnecessary. It's likelier that someone would use the WRONG address than the zero address, which we couldn't account for anyway. The real solution here is to make our deployment script really easy to configure and difficult to fuck up.

I would agree with you but skipping the check is not worth the gas gain for just a require done once at contract deployment

Issue 3: This may effect extensibility for little benefit.

Could you explain it? when the contract has been deployed every external function can be called by external contract. I agree that you should define them external only when the contract is "complete" and you have the overall picture

Issue 4: The real consideration here I think is making all our error messages constants so we don't have to compile them. Maybe some gas/code size savings to be explored here. Also standardizing error message formats to : could be wise. Let me know what you think.

As far I know there's no gas optimization in having those strings as constants (you were referring to that right?). Because anyway as soon as you have 1 char you're using gas cost of 256bits. Custom errors can be cheaper by a lot or at least at the same gas cost can be more UX friendly. I would love to make some repo with all these tests but I have an only a limited amount of time :(

issue 5: See my answer to issue 2.

Same answer to your answer to issue 2 ;)

issue 6: I can see the benefit, but I think passing a 0 value into the address to skip it is confusing UX. We can handle a lot of this with how we write all the scripts we package with it.

make sense

Issue 12: I think we can make a note to double check the system's ability to handle non-standard decimals. an internal _decimals() function with input/output control and doing 10 ** _decimals() whenever we need to normalize operations. Just making it dummy proof for people who fork the code.

yeah, I need more knowledge on that but I learned that you can't make any assumptions (or at least document them and be sure that everything you are interacting with follows those assumptions)

I didn't have a chance to follow all the tests so if you apply any of those suggestions (if they are changes to the sushi safe masterchef) be aware of that and cover them with extensive testing ;)

jaetask commented 2 years ago

Issue 4 Ref error messages.

I believe there was a plan to expose error messages such as ERROR_100, ERROR_101 etc.

_The strings ERROR_100 are just an example, we can go with any format that suits, or use constants like VALUE_NOTFOUND if this is preferred (as long as they are under 256bits).

We could publish the english translations in a JSON file along the lines of i18n/errors.en.json