Platform: Code4rena
Start Date: 07/01/2022
Pot Size: $80,000 USDC
Total HM: 21
Participants: 37
Period: 7 days
Judge: 0xean
Total Solo HM: 14
Id: 71
League: ETH
Rank: 7/37
Findings: 6
Award: $3,184.57
🌟 Selected for report: 9
🚀 Solo Findings: 2
1025.8688 INSURE - $359.05
622.8489 USDC - $622.85
p4st13r4
There is a typo in the unlock function, when setting the status of an insurance to false.
function unlock(uint256 _id) public { require( insurances[_id].status == true && marketStatus == MarketStatus.Trading && insurances[_id].endTime + parameters.getGrace(msg.sender) < block.timestamp, "ERROR: UNLOCK_BAD_COINDITIONS" ); insurances[_id].status == false; lockedAmount = lockedAmount - insurances[_id].amount; emit Unlocked(_id, insurances[_id].amount); }
Instead of doing:
insurances[_id].status = false;
The code just asserts an inequality, by using the == instead of =, like so:
insurances[_id].status == false;
This results in funds stuck and never really unlocked. Also, a malicious actor could bring the value of lockedAmount to zero by unlocking the same insurance multiple times, which would break the economy of the pool.
I don’t think a POC is needed, since the typo is evident: https://github.com/code-423n4/2022-01-insure/blob/main/contracts/PoolTemplate.sol#L360
Editor
Use assignment instead of comparison, as stated above
#0 - oishun1112
2022-01-19T09:38:25Z
🌟 Selected for report: p4st13r4
1139.8542 INSURE - $398.95
692.0543 USDC - $692.05
p4st13r4
Any user can pay the debt for any borrower in Vault.sol, by using repayDebt(). This function allows anyone to repay any amount of borrowed value, up-to and including the totalDebt value; it works by setting the debts[_target] to zero, and decreasing totalDebt by the given amount, up to zero. However, all debts of the other borrowers are left untouched.
If a malicious (but generous) user were to repay the debt for all the borrowers, markets functionality regarding borrowing would be DOSed: the vault would try to decrease the debt of the market, successfully, but would fail to decrease totalDebt as it would result in an underflow
https://github.com/code-423n4/2022-01-insure/blob/main/contracts/Vault.sol#L257
Editor
Make repayDebt() accept an amount up-to and including the value of the debt for the given borrower
#0 - oishun1112
2022-01-28T06:03:06Z
this needs to be specified how in more detail.
🌟 Selected for report: p4st13r4
379.9514 INSURE - $132.98
230.6848 USDC - $230.68
p4st13r4
PoolTemplate.sol and IndexTemplate.sol report this same error when trying to withdraw and some conditions are not met: "ERROR: WITHDRAWAL_PENDING”
However, PoolTemplate.sol does that when the marketStatus is not Trading; IndexTemplate.sol when the contract is locked. Since CDSTemplate.sol, instead, implement a different revert string, it’s best for understanding what revert strings are related to by making them as explicit and clear as possible. CDSTemplate.sol has this in the withdraw function:
require(paused == false, "ERROR: PAUSED");
https://github.com/code-423n4/2022-01-insure/blob/main/contracts/IndexTemplate.sol#L217
https://github.com/code-423n4/2022-01-insure/blob/main/contracts/PoolTemplate.sol#L302
Editor
Improve revert strings wording
🌟 Selected for report: p4st13r4
379.9514 INSURE - $132.98
230.6848 USDC - $230.68
p4st13r4
The resume function can be called by any user, at any time, even when the Index contract is not locked. There should be a check preventing it from being called unless the contract is locked
https://github.com/code-423n4/2022-01-insure/blob/main/contracts/IndexTemplate.sol#L459
Editor
Add a require on top:
require(locked);
37.3929 INSURE - $13.09
22.7028 USDC - $22.70
p4st13r4
The amount of the withdrawal request is not correctly updated after a withdrawal in CDSTemplate.sol. This happens because the withdrawal request is read from storage and put in memory, like this:
Withdrawal memory request = withdrawalReq[msg.sender];
However, the requested amount is not updated properly since the withdrawalReq in the storage is never updated. Instead, its in-memory version is updated, but it’s useless because that object is never used again:
//reduce requested amount request.amount -= _amount;
This issue is non critical because there is a function that takes care of updating the withdrawal requests’ amount on every token transfer: https://github.com/code-423n4/2022-01-insure/blob/main/contracts/CDSTemplate.sol#L358
The issue lies in the fact that the code seems to behave differently from how it looks at a first glance. Furthermore, the other two templates correctly update the value of the withdrawal request, so the version in CDSTemplate.sol should be aligned as well:
https://github.com/code-423n4/2022-01-insure/blob/main/contracts/CDSTemplate.sol#L230
Editor
Update the amount of the current withdrawal request as well
🌟 Selected for report: p4st13r4
62.2711 INSURE - $21.79
32.6923 USDC - $32.69
p4st13r4
When assigning the new owner, as well as emitting the event, msg.sender could be used to avoid reading from the storage
https://github.com/code-423n4/2022-01-insure/blob/main/contracts/Ownership.sol#L65
Editor
Change to:
function acceptTransferOwnership() external override onlyFutureOwner { /*** *@notice Accept a transfer of ownership */ _owner = msg.sender; emit AcceptNewOwnership(msg.sender); }
🌟 Selected for report: p4st13r4
62.2711 INSURE - $21.79
32.6923 USDC - $32.69
p4st13r4
When emitting the event, the function argument could be used, instead of reading from storage again
https://github.com/code-423n4/2022-01-insure/blob/main/contracts/Ownership.sol#L62
Editor
Change to:
emit CommitNewOwnership(newOwner);
🌟 Selected for report: p4st13r4
62.2711 INSURE - $21.79
32.6923 USDC - $32.69
p4st13r4
Users that incorrectly ask for a withdrawal equal to zero, will waste more gas (a storage read) since the check for amount > 0 is put after the check for the available amount
Editor
Move this require at the top of the requestWithdraw function:
require(_amount > 0, "ERROR: REQUEST_ZERO");
🌟 Selected for report: p4st13r4
62.2711 INSURE - $21.79
32.6923 USDC - $32.69
p4st13r4
totalAllocPoint in set() function is read several times from storage. It can be assigned to a local variable so the function is less expensive overall
https://github.com/code-423n4/2022-01-insure/blob/main/contracts/IndexTemplate.sol#L612
Editor
Assign totalAllocPoint to localTotalAllocPoint (or cachedTotalAllocPoint)
28.022 INSURE - $9.81
14.7115 USDC - $14.71
p4st13r4
In attributionValue() and underlyingValue(), respectively totalAttributions and attributions[_target] could be assigned locally and re-used later on, in order to save gas by preventing the same read from storage happening twice
https://github.com/code-423n4/2022-01-insure/blob/main/contracts/Vault.sol#L388
https://github.com/code-423n4/2022-01-insure/blob/main/contracts/Vault.sol#L406
Editor
#0 - 0xean
2022-01-28T02:23:08Z
dupe of #178
🌟 Selected for report: p4st13r4
62.2711 INSURE - $21.79
32.6923 USDC - $32.69
p4st13r4
Many functions that read params, check whether the value is set for the given target, otherwise return the value for the zero-address. When doing this kind of check, the value of the target is read twice:
These functions are used a lot of times inside all the contracts, so having them optimized as much as possible is required in order to save gas
Editor
Assign the target value and, if the check returns a value different from the zero-address, use it. For example, getFeeRate becomes:
function getFeeRate(address _target) external view override returns (uint256) { uint256 _targetFee = _fee[_target]; if (_targetFee == 0) { return _fee[address(0)]; } else { return _targetFee; } }
28.022 INSURE - $9.81
14.7115 USDC - $14.71
p4st13r4
In IndexTemplate.sol, the internal function _adjustAlloc has the following conditional statements:
if (_current > _target && _available != 0) { //if allocated credit is higher than the target, try to decrease uint256 _decrease = _current - _target; IPoolTemplate(_poolList[i].addr).withdrawCredit(_decrease); totalAllocatedCredit -= _decrease; } if (_current < _target) { //Sometimes we need to allocate more uint256 _allocate = _target - _current; IPoolTemplate(_poolList[i].addr).allocateCredit(_allocate); totalAllocatedCredit += _allocate; } if (_current == _target) { IPoolTemplate(_poolList[i].addr).allocateCredit(0); }
Instead of using all the if statements, they could be converted to if/else if/else in order to save gas avoiding useless computation, since all the branches are mutually exclusive
https://github.com/code-423n4/2022-01-insure/blob/main/contracts/IndexTemplate.sol#L339
Editor
As explained above, use if/else if/else conditions
#0 - oishun1112
2022-01-13T17:44:46Z
28.022 INSURE - $9.81
14.7115 USDC - $14.71
p4st13r4
The available() function is called three times in withdrawValue(), which performs two storage reads every time. The function could be called once, assigned to a local variable, and re-used later to avoid wasting gas
https://github.com/code-423n4/2022-01-insure/blob/main/contracts/Vault.sol#L163
Editor
Cache available() by assigning to e.g. availableBalance
#0 - oishun1112
2022-01-17T08:21:35Z
🌟 Selected for report: 0xngndev
Also found by: Dravee, Jujic, Meta0xNull, defsec, p4st13r4, robee, sirhashalot, tqts
2.9784 INSURE - $1.04
1.5637 USDC - $1.56
p4st13r4
Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met. Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc...
Many require calls contain long revert strings, e.g: https://github.com/code-423n4/2022-01-insure/blob/main/contracts/IndexTemplate.sol#L428
Editor
Shorten the revert strings to fit in 32 bytes
#0 - oishun1112
2022-01-13T17:47:41Z