🧾 Chainlink Proof of Reserve by pkuchtatt · Pull Request #1176 · trusttoken/contracts-pre22

pkuchtatt · 2022-05-16T19:27:33Z

No description provided.

tt-krzysztof · 2022-05-17T15:59:16Z

+    await token.setChainReserveFeed(mockV3AggregatorWith6Decimals.address)
+    expect(await token.chainReserveFeed()).to.equal(mockV3AggregatorWith6Decimals.address)
+    await token.setChainReserveHeartbeat(ONE_DAY_SECONDS)
+    expect(await token.chainReserveHeartbeat()).to.equal(ONE_DAY_SECONDS)


We don't need to verify every time that setting the heartbeat works. It's enough to verify it in one test.

Removed redundant checks.

tt-krzysztof · 2022-05-17T16:00:49Z

+  })
+
+  it('should revert if TrueCurrency supply > proof-of-reserves', async () => {
+    // Re-deploy aggregator with fewer TUSD in reserves


We'r not re-deploying here, just updating the oracle's answer.

Removed the comment.

tt-krzysztof · 2022-05-18T11:19:55Z

+        require(
+            chainReserveHeartbeat == 0 ||
+                (newHeartbeat != chainReserveHeartbeat && !(newHeartbeat == 0 && chainReserveHeartbeat == MAX_AGE)),
+            "TrueCurrency: new chainReserveHeartbeat must be different to current heartbeat"


Add test cases covering this condition.

Do we need this require? It shouldn't hurt to set chainReserveHeartbeat to the same value as before, and if we want an explicit MIN_AGE == 1 then we should add that as a constant -- although personally I don't see much difference between 1 second of heartbeat and 0 seconds for Chainlink's update frequency (at most once per block).

Added tests covering the condition.

@yuchenlintt That's a good question. The code was originally written by a Chainlink eng. It seems for some reason he thought that not allowing to set chainReserveHeartbeat to the same value was important. I can see this would lead to emission of NewChainReserveHeartbeat(<n>, <n>) events, which would be a little silly, but we could as well skip emitting instead of reverting in this case. What do you think?

I think it's fine to emit an event that says nothing changed. Anyone who consumes the event should be able to handle this without any issues.

In smart contracts I'd much prefer to reason about simple code than a bunch of edge cases. And in this example I don't see any failure modes that would force us to add this require (which might instead backfire and prevent us from newHeartbeat == 0 if for some reason we need to, for instance).

Committed suggestion.

yuchenlintt · 2022-05-19T11:36:11Z

🚨 I haven't had a chance to review most of this code yet, but a critical first step is that we need to first merge #1170 into main, followed by merging #1171 into main.

✂️ Disable delegation in TUSD #1170 fixes a critical vuln in mainnet TUSD that OpenZeppelin reported to us a few months ago.
🍼 Deploy TrueUSDNoDelegate to mainnet #1171 deploys this fixed contract to mainnet. This upgrade has already happened, so for this PR we can skip the final diff check that we'd typically do for changes to deployments-mainnet.json.

yosriady · 2022-05-19T11:43:32Z

@@ -0,0 +1,20 @@
+// SPDX-License-Identifier: MIT
+pragma solidity 0.6.10;


Can we update to the latest Solidity version?

Unfortunately probably not, since the existing contract has a lot of legacy code that we would need to inspect carefully for upgrade compatibility.

yosriady · 2022-05-19T11:54:23Z

+     * @dev Admin function to set a new feed
+     * @param newFeed Address of the new feed
+     */
+    function setChainReserveFeed(address newFeed) external override onlyOwner returns (uint256) {


Does this function need a zero address check?

I would recommend against a zero address check, since address(0) in this current implementation acts as a sentinel value that indicates proof of reserve is disabled. I would be more amenable to adding a zero address check if there were no special case logic for chainReserveFeed == address(0).

Setting to 0 might be necessary to call in case, for instance, the Chainlink aggregator goes down for a period that is longer than MAX_AGE.

We need to be able to set chainReserveFeed to 0 to disable temporarily the PoR check in case Armanino API or Chainlink is down for an extended period of time.

yosriady · 2022-05-19T11:58:18Z

+abstract contract TrueCurrencyWithPoR is TrueCurrency, IPoRToken {
+    using SafeMath for uint256;
+
+    uint256 public constant MAX_AGE = 7 days;


Will MAX_AGE never need to change?

Not sure. To be honest I'm not sure having MAX_AGE is needed at all, maybe it would be better to allow contract owner to set chainReserveHeartbeat to whatever he wants. What do you think? CC @yuchenlintt , @tt-krzysztof

Agree, I don't think it's necessary to have a MAX_AGE set. The setter is only callable by owner anyway, which for TUSD is the same as the proxyOwner that can override MAX_AGE via upgrade.

Reworked the code so chainReserveHeartbeat is initialised in the constructor to 7 days but can be later reset to any value.

yuchenlintt · 2022-05-19T12:11:08Z

+    uint256 public constant MAX_AGE = 7 days;
+
+    constructor() public {
+        chainReserveHeartbeat = MAX_AGE;


If we're doing an upgrade of TUSD, then this constructor won't get called. We'll have to tell Techteryx to queue setChainReserveHeartbeat(7 days) from the multisig after the upgrade call.

Edit: the order things are called is important! If Techteryx executes setChainReserveFeed() before setChainReserveHeartbeat(), then all mints will be rejected until heartbeat is high enough, since updatedAt will need to be at or above block.timestamp

If this isn't intended to change very often, another possibility would be to hardcode both CHAIN_RESERVE_HEARTBEAT and CHAIN_RESERVE_FEED as compiled constants. This would let us skip the setter logic and just deploy an upgrade whenever it's necessary to reset these.

Given communication delays with Techteryx, I think it's easier to ask them to queue a single upgrade than to request an ordered sequence of function calls.

As mentioned earlier, I think MAX_AGE is an arbitrary constant, I don't think anybody knows for sure what a really useful/practical MAX_AGE is at the moment. I think it's probably best not to constrain the possible values of chainReserveHeartbeat in the code and remove MAX_AGE altogether. Please do let me know if you think otherwise.

We need to be able to disable temporarily PoR if Armanino, Chainlink or one of our banking partners' API (most likely) is down for an extended period of time, this will be done be setting chainReserveFeed to 0. We should be able to do it quickly, e.g. in case of a VIP mint, and I think calling setChainReserveFeed should be quicker and cheaper than upgrading the contract. Do you agree?

Also, we plan to deploy it on Ethereum and Avalanche, which, I assume, will have different feed addresses, so I think hardcoding CHAIN_RESERVE_FEED in the code is probably not a good idea.

Having the option to set chainReserveFeed to 0 sounds to me like enough reason to keep this as a storage variable! And yeah, I'd overlooked that Ethereum mainnet and Avalanche will have different addresses. Definitely shouldn't hardcode in that case.

yuchenlintt · 2022-05-19T12:27:49Z

+        uint256 oldestAllowed = block.timestamp.sub(chainReserveHeartbeat, "TrueCurrency: Invalid timestamp from PoR feed");
+        require(updatedAt >= oldestAllowed, "TrueCurrency: PoR answer too old");


Unless MAX_AGE > 53 years or something seriously wrong happened to block.timestamp, I wouldn't bother to add an error message for uint underflow on a Unix timestamp.

Suggested change

uint256 oldestAllowed = block.timestamp.sub(chainReserveHeartbeat, "TrueCurrency: Invalid timestamp from PoR feed");

require(updatedAt >= oldestAllowed, "TrueCurrency: PoR answer too old");

require(updatedAt >= block.timestamp.sub(chainReserveHeartbeat), "TrueCurrency: PoR answer too old");

Good point! It made me realise that we do have to constrain values of chainReserveHeartbeat. Added MAX_CHAIN_RESERVE_HEARTBEAT and set it to 30 days, it's hopefully more than we will never need but still not cause the overflow here. Note, INITIAL_CHAIN_RESERVE_HEARTBEAT is set to 7 days now.

yuchenlintt · 2022-05-19T12:38:40Z

+     * @param newFeed Address of the new feed
+     */
+    function setChainReserveFeed(address newFeed) external override onlyOwner returns (uint256) {
+        require(newFeed != chainReserveFeed, "TrueCurrency: new chainReserveFeed must be different to current feed");


Do we need this require? I don't think it hurts to set chainReserveFeed to the same value.

I would consider skiping the storage write and event emission if it's set to the same value:

if (newFeed != chainReserveFeed) { ... }

Removed require and added ifs as suggested to avoid writing to storage and emitting events when no change.

yuchenlintt

(Possibly in a follow-up PR) it's a good idea to add a simulation test that forks the mainnet contract address 0x0000000000085d4780B73119b644AE5ecd22b376, performs the upgrade, and checks that a user is still able to mint, redeem, etc.

pkuchtatt · 2022-05-19T14:30:06Z

🚨 I haven't had a chance to review most of this code yet, but a critical first step is that we need to first merge #1170 into main, followed by merging #1171 into main.

✂️ Disable delegation in TUSD #1170 fixes a critical vuln in mainnet TUSD that OpenZeppelin reported to us a few months ago.

🍼 Deploy TrueUSDNoDelegate to mainnet #1171 deploys this fixed contract to mainnet. This upgrade has already happened, so for this PR we can skip the final diff check that we'd typically do for changes to deployments-mainnet.json.

@yuchenlintt Thank you for the info! Please let us know when this is ready. Speaking about the repo, I noticed CI always fails on the verify task (certora). Do you have an idea why? If not, do you think we can remove the task from CI?

yuchenlintt · 2022-05-19T14:39:35Z

@yuchenlintt Thank you for the info! Please let us know when this is ready.

These PRs have been ready to merge since February, but no one's approved them :)

I noticed CI always fails on the verify task (certora). Do you have an idea why? If not, do you think we can remove the task from CI?

My guess is our dev environment is probably out of date? Calls to Certora go to an external service, so if we haven't updated our client version then there might be some errors. Feel free to remove this check from CI -- we'd only gotten around to verifying code that has since become obsolete.

aiagentdev-1990 · 2022-05-19T14:43:29Z

+
+        // Get latest proof-of-reserves from the feed
+        (, int256 signedReserves, , uint256 updatedAt, ) = IChainlinkAggregatorV3(chainReserveFeed).latestRoundData();
+        require(signedReserves > 0, "TrueCurrency: Invalid answer from PoR feed");


Just a small suggestion to save gas but maybe we can shorten the require messages

bryanjowers · 2022-05-20T17:16:47Z

Thanks @yosriady and @cds95 for taking a quick look over this PR. @pkuchtatt Glad we could provide a few helpful bits of feedback, but want to call out this shouldn't be considered a complete review or audit. excited about this project!

yuchenlintt · 2022-07-13T16:05:54Z

+abstract contract TrueCurrencyWithPoR is TrueCurrency, IPoRToken {
+    using SafeMath for uint256;
+
+    uint256 public constant MAX_CHAIN_RESERVE_HEARTBEAT = 30 days;


I think we had agreed it would be cleaner to remove MAX_AGE, which I believe has been renamed to MAX_CHAIN_RESERVE_HEARTBEAT:
#1176 (comment)

I might have missed the conversation where we decided to keep it, though.

So, I realised here #1176 (comment) that we should constrain the values of chainReserveHeartbeat because it can potentially cause an underflow in this code:

require(updatedAt >= block.timestamp.sub(chainReserveHeartbeat), "TrueCurrency: PoR answer too old");

I think it's better to revert with a clear error message that will help in debugging rather than potentially allow the code to fail with an underflow. What do you think?

Ah, thanks for finding the context! Sorry I missed your reply from May.

I would instead rephrase the require statement like this:

require(block.timestamp.sub(updatedAt) <= chainReserveHeartbeat, "blah reasons")

It should be nearly impossible to underflow on this require since updatedAt presumably gets set to block.timestamp from a previous tx, and the chain enforces monotonicity of timestamps.

There's also a benefit that you can set extreme values for chainReserveHeartbeat like MAX_UINT256, and this code won't unexpectedly revert.

Great idea! updatedAt is an answer from chainlink aggregator contract, I imagine they may be scenarios where we get a wrong value here, e.g. when owner sets chainReserveFeed to a wrong/malicious contract, and it's causing an underflow, however we can and probably should protect ourselves from this with something like

require(block.timestamp >= updatedAt)

Will implement this shortly, sorry about the delay, was setting up a new workstation and also found that an integration test is failing on the main branch, here is a PR with a fix: #1180 Can you please review and merge?

As discussed, reworked the require statement and removed MAX_CHAIN_RESERVE_HEARTBEAT .

yuchenlintt · 2022-07-13T16:14:33Z

+        require(newHeartbeat <= MAX_CHAIN_RESERVE_HEARTBEAT, "TrueCurrency: PoR heartbeat too long");
+


Delete? This function is onlyOwner, and the owner can make any arbitrary upgrade already anyway, so this require doesn't actually prevent them from doing anything.

Suggested change

require(newHeartbeat <= MAX_CHAIN_RESERVE_HEARTBEAT, "TrueCurrency: PoR heartbeat too long");

As explained above, too big maxChainReserveHeartbeat can cause an underflow in one of the requires.

Here's my reply: #1176 (comment)

In brief: the only possible way for my proposal to underflow would be if block.timestamp < updatedAt, which should not happen assuming block.timestamp is monotonic increasing. If this change means we can cut out extra code, I'd be happy to accept a slightly less legible error in that situation.

Co-authored-by: yuchenlintt <80530881+yuchenlintt@users.noreply.github.com>

…underflow for high values

yuchenlintt

LGTM, thanks for carrying this through!

For the sake of a cleaner commit history, I suggest we revert the irrelevant changes and handle them in a separate PR. I'll commit these as suggested changes and then approve this PR.

yuchenlintt · 2022-08-08T15:39:25Z

#1170 and #1171 have been merged, so this should be ready as well now!

pkuchtatt added 3 commits May 16, 2022 20:24

Update README.md: smart-contracts => contracts-pre22

3db925f

Fix test TrueFiPool2

cf8584c

Chainlink Proof of Reserve mint check

588d629

pkuchtatt requested a review from tt-krzysztof May 17, 2022 11:25

tt-krzysztof reviewed May 17, 2022

View reviewed changes

tt-krzysztof reviewed May 18, 2022

View reviewed changes

Comment thread test/true-currencies/TrueCurrencyWithPoR.test.ts Outdated

tt-krzysztof reviewed May 18, 2022

View reviewed changes

Comment thread contracts/true-currencies/TrueCurrencyWithPoR.sol Outdated

tt-krzysztof reviewed May 18, 2022

View reviewed changes

Comment thread contracts/true-currencies/TrueCurrencyWithPoR.sol Outdated

Address review comments

70e6630

tt-krzysztof reviewed May 18, 2022

View reviewed changes

Comment thread contracts/true-currencies/TrueCurrencyWithPoR.sol

pkuchtatt added 8 commits May 18, 2022 12:47

Fix test

96ecb47

Remove redundant check

05db2de

Address review comments: use timeTravel

ba73302

Address review comments

0887343

Fix typecheck

f99aaa7

Address review comments: add test

c09157a

Address review comments: add test

8e51389

Fix linter issue

c9e9f5a

yosriady reviewed May 19, 2022

View reviewed changes

yuchenlintt reviewed May 19, 2022

View reviewed changes

pkuchtatt added 4 commits May 19, 2022 13:54

Address review comments: add tests

e5cb35b

Address review comment

b689030

Address review comment

6271d7d

Address review comment

bc6e59b

aiagentdev-1990 reviewed May 19, 2022

View reviewed changes

pkuchtatt added 5 commits May 20, 2022 12:36

Address review comments

9c6d025

Address review comments

cc36153

Address review comments

3a4cfbe

Fix linter issue

bfc5e3b

Optimize storage

95f8a05

Address review comments. MAX_CHAIN_RESERVE_HEARTBEAT = 30 days

80cf1bf

pkuchtatt mentioned this pull request May 23, 2022

🔇 Disable Certora in Circle CI #1178

Merged

yuchenlintt requested changes Jul 13, 2022

View reviewed changes

pkuchtatt and others added 8 commits July 14, 2022 12:18

Update contracts/true-currencies/TrueCurrencyWithPoR.sol

ba3d72d

Co-authored-by: yuchenlintt <80530881+yuchenlintt@users.noreply.github.com>

Update contracts/true-currencies/TrueCurrencyWithPoR.sol

420bbf4

Co-authored-by: yuchenlintt <80530881+yuchenlintt@users.noreply.github.com>

Merge branch 'main' into proof-of-reserve2

3da7764

Merge branch 'main' into proof-of-reserve2

c3b09c6

Correct automatic merge

782576f

Rework check if answer not older than chainReserveHeartbeat to avoid …

49a98fe

…underflow for high values

Remove MAX_CHAIN_RESERVE_HEARTBEAT

21460b8

Removed no longer needed test

af95cf3

yuchenlintt reviewed Jul 22, 2022

View reviewed changes

Comment thread test/integration/TrueFiPool2.test.ts

Comment thread README.md Outdated

Comment thread README.md Outdated

Comment thread README.md Outdated

Revert irrelevant diffs in this PR; can be submitted separately

829e27b

yuchenlintt approved these changes Jul 22, 2022

View reviewed changes

tt-krzysztof approved these changes Jul 26, 2022

View reviewed changes

yuchenlintt merged commit 6db38fc into main Aug 8, 2022

yuchenlintt deleted the proof-of-reserve2 branch August 8, 2022 15:39

yuchenlintt mentioned this pull request Aug 24, 2022

💯 Add TUSD PoR decimal vuln proof of concept unit test #1185

Closed

		@@ -0,0 +1,20 @@
		// SPDX-License-Identifier: MIT
		pragma solidity 0.6.10;

		uint256 oldestAllowed = block.timestamp.sub(chainReserveHeartbeat, "TrueCurrency: Invalid timestamp from PoR feed");
		require(updatedAt >= oldestAllowed, "TrueCurrency: PoR answer too old");

	uint256 oldestAllowed = block.timestamp.sub(chainReserveHeartbeat, "TrueCurrency: Invalid timestamp from PoR feed");
	require(updatedAt >= oldestAllowed, "TrueCurrency: PoR answer too old");
	require(updatedAt >= block.timestamp.sub(chainReserveHeartbeat), "TrueCurrency: PoR answer too old");

		require(newHeartbeat <= MAX_CHAIN_RESERVE_HEARTBEAT, "TrueCurrency: PoR heartbeat too long");

Uh oh!

Conversation

pkuchtatt commented May 16, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yuchenlintt commented May 19, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuchenlintt May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuchenlintt May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuchenlintt May 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuchenlintt left a comment

Choose a reason for hiding this comment

Uh oh!

pkuchtatt commented May 19, 2022

Uh oh!

yuchenlintt commented May 19, 2022

Uh oh!

Uh oh!

yuchenlintt May 19, 2022 •

edited

Loading

yuchenlintt May 19, 2022 •

edited

Loading

yuchenlintt May 19, 2022 •

edited

Loading