Open c4-bot-9 opened 4 months ago
piske-alex (sponsor) confirmed
alcueca marked the issue as satisfactory
alcueca marked the issue as selected for report
This issue is invalid. When the program is deployed in production it will use the release
flag: arithmetics underflow/overflow only in debug mode, that's why the test works in this PoC.
Run it again with --release
and you will see that the program will not panic, and an underflow will actually occur. Even if it underflows, this hasn't any impact as total_borrowed
isn't used anywhere, and it's not possible to DoS the position.
Here are the docs, please re-check.
If the overflow-checks
flag is set to true, then arithmetic under/overflows will also revert in release mode (you can read more about that here). This issue also seems to share the same root cause as #30, its just that their described impacts are different.
@Arabadzhiew you are right, my bad. But doesn't that mean that #30 and other similar issues that assume a successful underflow/overflow that won't panic are not valid? As it will behave like you described here
@DadeKuma Given that overflow-checks
is set to true
in the Cargo.toml
file, you are right, the impact described in this issue is the only valid one.
However, I am not sure how the rest of the issues should be judged, as they still reference the same root cause, although their described impacts are wrong. I'll leave that up to @alcueca to decide.
As @Arabadzhiew said, when overflow-checks is set to true, panic will be the behaviour. However, here are reasons why I reported the other issue #30
Cargo.toml
is not in scope. Therefore, any configuration that effects the program shouldn’t be considered. It's not a common practice to enable overflow-checks anyway. This is similar to this issue phala-network#65 (from a different Rust contest) that I reported but was considered out of scope eventually, since the bug exited in Cargo.lock
which was out of scope.
I believe overflow-checks isn't set intentional, because in the code, checked functions are used (e.g. checked_mul
and checked_div
). If overflow-checks is set to true, why would you use checked functions?
CC: @alcueca
@koolexcrypto So we shouldn't take the protocol configuration into account during contests? That doesn't make much sense to me, but well, if the rules say so, then so be it.
I believe overflow-checks isn't set intentional, because in the code, checked functions are used (e.g. checked_mul and checked_div). If overflow-checks is set to true, why would you use checked functions?
There are also calls to checked_sub
on swapback.rs#L150 which do default to 0 in the case of an underflow. How should we interpret those? IMO, they are clearly showing the intent of the protocol team to use the overflow checks provided by Rust in their production deployment. Also, for the calls to checked_mul
and checked_div
, an argument can be made that they are simply used for having custom error messages in the event where the overflow checks are enabled.
@koolexcrypto
Therefore, any configuration that effects the program shouldn’t be considered
I think this contradicts what you are saying in #15. But if this is true, that means #15 is valid no matter the version of SPL used, as we can assume that they can use a version without safety checks (and also in that case the sponsor added some safety checks in one part of the code, but others were missing)
To be consistent, I'm going to state that cargo.toml is out of scope, and that the program will underflow without reverting. total_borrows
will have an incorrect value, that as judged elsewhere, it has no impact on the protocol and makes the severity QA.
A recommendation for future Solana/Rust contests is to include configuration files in the scope, and be clear about the release configuration.
alcueca changed the severity to QA (Quality Assurance)
alcueca marked the issue as grade-a
alcueca marked the issue as not selected for report
Lines of code
https://github.com/code-423n4/2024-04-lavarage/blob/9e8295b542fb71b2ba9b4693e25619585266d19e/libs/smart-contracts/programs/lavarage/src/processor/swapback.rs#L197
Vulnerability details
Impact
The last positions from a given pool will become permanently non-closable, preventing borrowers from claiming back their collateral and repaying their loans, leading to financial losses for them (since all positions are over-collateralized by default)
Proof of Concept
The current implementation of the
repay_sol
function allows it to be called an infinite number of times for positions that have already been closed. However, if we take a look at the following snippet from the function:We can see that the
total_borrowed
value of the node wallet of the trading pool of the position is being decreased byborrowed_amount
on each call to it. What this means is that in the event where the function was to be called more than once for any position, no matter how big or small it is, it will lead to the function always reverting for the last position/s of the given trading pool, as the subtraction fromtotal_borrowed
will always revert due to an arithmetic underflow. This can easily be abused by opening dust-amount positions and then callingrepay_sol
multiple times for them.The following coded PoC demonstrates the issue at question. To run it, paste it after the
Should set maxBorrow
test in thelavarage
describe block in thelavarage.spec.ts
test file and then runanchor test
inlibs/smart-contracts
. The tests insidelavarage.spec.ts
are stateful, so the order in which they are executed does matter.Tools Used
Manual review
Recommended Mitigation Steps
Use
checked_sub
for subtracting theborrowed_amount
from thetotal_borrowed
value, and default to 0 in the case of an underflow:Assessed type
Under/Overflow