Fix/chainlight remediations

[livingrockrises]

Summary

Related Issue: #
https://github.com/chainlight-io/2024-01-biconomy-audit-issues-client/issues/7
https://github.com/chainlight-io/2024-01-biconomy-audit-issues-client/issues/5
https://github.com/chainlight-io/2024-01-biconomy-audit-issues-client/issues/6
https://github.com/chainlight-io/2024-01-biconomy-audit-issues-client/issues/7

Change Type

• Bug Fix
• Refactor
• New Feature
• Breaking Change
• Documentation Update
• Performance Improvement
• Other

Checklist

• My code follows this project's style guidelines
• I've reviewed my own code
• I've added comments for any hard-to-understand areas
• I've updated the documentation if necessary
• My changes generate no new warnings
• I've added tests that prove my fix is effective or my feature works
• All unit tests pass locally with my changes
• Any dependent changes have been merged and published

Additional Information

Branch Naming

[this-is-chainlight]

https://github.com/chainlight-io/2024-01-biconomy-audit-issues-client/issues/5

Looks good.

---

https://github.com/chainlight-io/2024-01-biconomy-audit-issues-client/issues/6

1. Most oracles are still configured to fall back to the default update threshold of 2 days. Since the heartbeat interval varies for each token and network combination, the priceUpdateThreshold should be configured as heartbeat interval + block time + buffer. You can find the information on Chainlink's feed here.

2. Placing priceUpdateThreshold right after tokenDecimals here and loading tokenInfo into memory before using it here should reduce gas usage.

3. In line with the code changes, comments should be added here and updated here.

---

https://github.com/chainlight-io/2024-01-biconomy-audit-issues-client/issues/7

There is a flaw in MathLib.sol's min function. (It returns the max value rather than the min value.) Change the gt() part in minuint256() and minuint32() as shown below.

function minuint256(uint256 a, uint256 b) internal pure returns (uint256 result) {
    assembly {
        result := xor(b, mul(xor(b, a), gt(b, a)))
    }
}

function minuint32(uint32 a, uint32 b) internal pure returns (uint32 result) {
    assembly {
        result := xor(b, mul(xor(b, a), gt(b, a)))
    }
}

[livingrockrises]

1. here

can't open this link
https://data.chain.link/_next/data/_iNK4_aT-_RWKGZ3A8gi7/feeds.json

[livingrockrises]

2. here

do you mean changing this from storage to memory?

TokenInfo storage tokenInfo = tokensInfo[token];

[livingrockrises]

There is a flaw in MathLib.sol's min function. (It returns the max value rather than the min value.) Change the gt() part in minuint256() and minuint32() as shown below.

Are you sure about this, it does break my test cases.

[livingrockrises]

There is a flaw in MathLib.sol's min function. (It returns the max value rather than the min value.) Change the gt() part in minuint256() and minuint32() as shown below.

Are you sure about this, it does break my test cases.

ok I just confirmed in foundry, will check the tests

[this-is-chainlight]

#81 (comment)

Sorry. We didn't know that the link was temporary. We are uploading the downloaded version.
feeds.json

#81 (comment)

Yes.

#81 (comment)

Yes.

[livingrockrises]

#81 (comment)

Sorry. We didn't know that the link was temporary. We are uploading the downloaded version. feeds.json

#81 (comment)

Yes.

#81 (comment)

Yes.

Thanks. For the deployment scripts will try to fin heartbeat for all supported oracles across supported chains. What value do you recommend for a buffer?

btw, all the changes have been made. can you re-review and approve the PR please

[this-is-chainlight]

What value do you recommend for a buffer?

buffer = max(heartbeat interval, average block time of chain * 2)

So the final threshold would be heartbeat interval * 2 + average block time of the chain or heartbeat interval + average block time of the chain * 3).

btw, all the changes have been made. can you re-review and approve the PR please

Sure. Looks good.

[Aboudjem]

Not sure _amount should be indexed

[livingrockrises]

we can index 3 params and I think it's fine. also out of scope of remediations PR

[Aboudjem]

To fit into 32 bytes we can use the following require msg: BTPM: Insufficient token balance

[livingrockrises]

done

[Aboudjem]

Current function setTokenOracle updates TokenInfo in tokensInfo map separately.

I'll suggest to pass TokenInfo directly and update map once. Saves SSTORE operation

tokensInfo[token] = TokenInfo({
    tokenDecimals: ERC20(token).decimals(),
    priceUpdateThreshold,
    tokenOracle,
    nativeOracle,
    isDerivedFeed
});

This way we use less gas

[livingrockrises]

can we do this as part of another PR? first I will check gas consumptions on forge. this also requires major changes again in scripts

[Aboudjem]

.