search

sherlock-audit / 2023-02-carapace-judging

2 stars 0 forks source link

Allarious - If a `lendingPool` is added to the network while in `late` state, can be defaulted instantly #151

Open github-actions[bot] opened 1 year ago

github-actions[bot] commented 1 year ago

Allarious

high

If a lendingPool is added to the network while in late state, can be defaulted instantly

Summary

If a lending pool is added to a protection pool, the defaultStateManager sets the currentState to late without setting the late timestamp. This can enable anyone in the network to be able to call the _assessState once more and mark the pool as default.

Vulnerability Detail

defaultStateManager user _assessState function to transfer between states. However, in case an underlying pool is called by _assessState for the first time when it is added to the protocol. The _assessState function sets the currentState to late without updating the lateTimestamp which will remain zero. The attacker can exploit this to move the pool to the default state where it locks the lending pool and renders it unusable.

While it is checked that when pools are added to the ReferenceLendingPool inside _addReferenceLendingPool that the pools should be in Active state, if in the time between the addition of a pool and the first time call of _assessState the pool goes from Active to Late, this attack can be performed by the attacker.

https://github.com/sherlock-audit/2023-02-carapace/blob/main/contracts/core/DefaultStateManager.sol#L370-L375

Impact

An attacker can render an underlying lending pool unusable.

Code Snippet

Tool used

Manual Review

Recommendation

The _assessState should handle the initial setting of the state seperately.

clems4ev3r commented 1 year ago

@vnadoda a lending pool cannot be added without being in the active state per this check: https://github.com/sherlock-audit/2023-02-carapace/blob/main/contracts/core/pool/ReferenceLendingPools.sol#L284 this sounds invalid

vnadoda commented 1 year ago

@clems4ev3r A reporter has mentioned that, but his main concern is the following: "While it is checked that when pools are added to the ReferenceLendingPool inside _addReferenceLendingPool that the pools should be in Active state, if in the time between the addition of a pool and the first time call of _assessState the pool goes from Active to Late, this attack can be performed by the attacker."

So I think this concern is valid, but low priority because the probability of this happening is very low.

vnadoda commented 1 year ago

@clems4ev3r We are planning to call DefaultStateManager.assessState just after adding the lending pool in op script. But maybe we should have addLendingPool in DefaultStateManager for onlyOwner, which should call RLP.add and then asset state as well? In any case, this is a low-priority issue, do you agree?

clems4ev3r commented 1 year ago

@vnadoda as discussed on call, given the steps taken during op script, and low probability of the issue this could be lowered in severity to low

hrishibhat commented 1 year ago

Considering this issue as low based on the comments above.

Allarious commented 1 year ago

Escalate for 75 USDC

The issue is pointing out that while pools get checked to be active when they are being added to the pool, they are not getting checked first time that they are undergoing the assessState call. This is a simple issue in nature which is incorrectly handled by the protocol that can lead to an incorrect lending pool state. Relying so heavily on assessState running at the same time each day can not be a good design choice and we see here that for every hour of delay, the pool is risking the default of underlying pools. Therefore, this should definitely be considered for at least medium severity. While the issue is explaining the simple attack well enough, here is a more extensive version of explanation.

This issue arises from two valid points that happen in the system:

The protocol expects addProtocol and assessState to happen together, which might make sense from the developer point of view, but opens up a serious attack vector from security point of view when protocol is running assessState once a day.

Also, naming could be changed to "If a lendingPool is added to the network while close to late state, can be defaulted instantly". I agree that writing and the naming could be much better but the issue is pointing out a valid problem.

Step by Step

(1) A pool in its active state gets added to the pool by calling _addReferenceLendingPool (2) Lets imagine the case where the daily assessState was just called (3) The lendingPool goes from Active -> Late before next assessState (4) In case where assessState is called the next day with any delay, the protocol will pass the grace period and enter the Late state. The protocol is not designed to handle this case, if the grace period was any smaller than one day, this issue could have caused much more damage. (5) When assessState is called, the defaultStateManager takes the system from Not supported -> Late without setting the lateTimestamp. (6) Any attacker in the system can call the assessState again to wrongfully default the pool

Justification of this issue being H/M

The issue is already showing sufficient data about the exploit, why it happens and how to stop it. Therefore, I think it should be labelled Medium or High.

Comments provided by LSW and Sponsor

@vnadoda a lending pool cannot be added without being in the active state per this check: https://github.com/sherlock-audit/2023-02-carapace/blob/main/contracts/core/pool/ReferenceLendingPools.sol#L284 this sounds invalid

This issue is talking about when the pool is assessed for the first time while in the late state. While I agree that the naming and writing could be better, it is still showing enough data if you read all of the issue carefully.

So I think this concern is valid, but low priority because the probability of this happening is very low.

The probability is considerable, as shown above. It is not a good idea to be relying on a script running everyday without no pre-cautions, now that we can see even hours late can cause some pools to default. Handling such cases on-chain would happen easily by correctly implementing Not Supported -> Late in defaultStateManager, while it is not handled correctly, should be heavily considered by the team.

We are planning to call DefaultStateManager.assessState just after adding the lending pool in op script.

If there is a need to handle this in a script/third contract, then it means there is a valid bug here. As stated in the initial scope of the audit, assessState gets called daily and _addReferenceLendingPool can be called anytime. I believe the issue's severity should be judged based on assessState running daily. However, if you want to use an external script/contract to handle this, you need to consider the points mentioned below.

sherlock-admin commented 1 year ago

Escalate for 75 USDC

The issue is pointing out that while pools get checked to be active when they are being added to the pool, they are not getting checked first time that they are undergoing the assessState call. This is a simple issue in nature which is incorrectly handled by the protocol that can lead to an incorrect lending pool state. Relying so heavily on assessState running at the same time each day can not be a good design choice and we see here that for every hour of delay, the pool is risking the default of underlying pools. Therefore, this should definitely be considered for at least medium severity. While the issue is explaining the simple attack well enough, here is a more extensive version of explanation.

This issue arises from two valid points that happen in the system:

  • assessState gets called daily.
  • The defaultStateManager does not handle Not Supported -> Late.

The protocol expects addProtocol and assessState to happen together, which might make sense from the developer point of view, but opens up a serious attack vector from security point of view when protocol is running assessState once a day.

Also, naming could be changed to "If a lendingPool is added to the network while close to late state, can be defaulted instantly". I agree that writing and the naming could be much better but the issue is pointing out a valid problem.

Step by Step

(1) A pool in its active state gets added to the pool by calling _addReferenceLendingPool (2) Lets imagine the case where the daily assessState was just called (3) The lendingPool goes from Active -> Late before next assessState (4) In case where assessState is called the next day with any delay, the protocol will pass the grace period and enter the Late state. The protocol is not designed to handle this case, if the grace period was any smaller than one day, this issue could have caused much more damage. (5) When assessState is called, the defaultStateManager takes the system from Not supported -> Late without setting the lateTimestamp. (6) Any attacker in the system can call the assessState again to wrongfully default the pool

Justification of this issue being H/M

  • (Considerable Probability) The probability of the pool going from Active -> Late within two assessStates are very high, however, the protocol is heavily relying on the assessState being run at the same time everyday and can be extremely dangerous. If we consider a pool's payment period is 14 days, possibility of a added pool to be instantly defaulted is (number_of_delay_hours/24) * 1/14, where for 6 hours of delay in running assessState, puts one out of roughly 50 added pools in danger of default.
  • (No Permission Needed) Anyone in the network can trigger the default, and have time as far as the underlying pool is at late state
  • (No Fees) Attacker needs to only pay the gas fee in order to cause the pool to default
  • (Complete DoS) The defaulted pool is unusable and can not be removed from the referenceLendingPools and would be marked defaulted, effectively DoSing the buyers of the defaulted pool.
  • (Inevitability) Lets imagine you run the script between 12:00 pm and 01:00 pm each day and therefore you have around 15 minutes delay time on average in 90 days, you will have an overall of 22.5 hours overall delay and if we consider the probability uniform, the change of default of pools is (22.5/24) * 1/active_period_in_days which is very close to 1/active_period_in_days. if active_period_in_days = 14 then you have one out of 14 default because of this.

The issue is already showing sufficient data about the exploit, why it happens and how to stop it. Therefore, I think it should be labelled Medium or High.

Comments provided by LSW and Sponsor

@vnadoda a lending pool cannot be added without being in the active state per this check: https://github.com/sherlock-audit/2023-02-carapace/blob/main/contracts/core/pool/ReferenceLendingPools.sol#L284 this sounds invalid

This issue is talking about when the pool is assessed for the first time while in the late state. While I agree that the naming and writing could be better, it is still showing enough data if you read all of the issue carefully.

So I think this concern is valid, but low priority because the probability of this happening is very low.

The probability is considerable, as shown above. It is not a good idea to be relying on a script running everyday without no pre-cautions, now that we can see even hours late can cause some pools to default. Handling such cases on-chain would happen easily by correctly implementing Not Supported -> Late in defaultStateManager, while it is not handled correctly, should be heavily considered by the team.

We are planning to call DefaultStateManager.assessState just after adding the lending pool in op script.

If there is a need to handle this in a script/third contract, then it means there is a valid bug here. As stated in the initial scope of the audit, assessState gets called daily and _addReferenceLendingPool can be called anytime. I believe the issue's severity should be judged based on assessState running daily. However, if you want to use an external script/contract to handle this, you need to consider the points mentioned below.

  • Either you are using a third smart contract to call _addReferenceLendingPool and assessState atomically, which is the safest option.
  • Or you are using an off-chain script to send two transactions _addReferenceLendingPool and assessState together, where in this case, if the transactions are not included in the certain order of _addReferenceLendingPool first and assessState second, the assessState would be ineffective and attack is again valid.

You've created a valid escalation for 75 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

hrishibhat commented 1 year ago

Escalation accepted

Based on internal discussions with the Team and the Lead Watson, the protocol calls for assessstate as soon as the lending pool is added, but this information was not available during the audit, there is a likely chance the issue could still happen, hence considering this a valid medium.

sherlock-admin commented 1 year ago

Escalation accepted

Based on internal discussions with the Team and the Lead Watson, the protocol calls for assessstate as soon as the lending pool is added, but this information was not available during the audit, there is a likely chance the issue could still happen, hence considering this a valid medium.

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.

vnadoda commented 1 year ago

@clems4ev3r fix for this issue: https://github.com/carapace-finance/credit-default-swaps-contracts/pull/74

clems4ev3r commented 1 year ago

Fix looks good