Platform: Code4rena
Start Date: 24/02/2022
Pot Size: $170,000 UST
Total HM: 15
Participants: 16
Period: 14 days
Judge: Albert Chon
Total Solo HM: 11
Id: 82
League: COSMOS
Rank: 9/16
Findings: 2
Award: $4,319.20
🌟 Selected for report: 1
🚀 Solo Findings: 0
This audit contest is probably the largest in codebase size to date, consisting of various components such as tokens, airdrop, governance, staking, liquidity mining, reward distributions, a money market and cross-chain bridge functionality. While these components aren’t complex when reviewed in isolation, the code complexity becomes a little higher because of the various interactions with each other.
Overall, the codebase quality is very high. Inline comments provided are helpful in describing why some checks were not performed (mainly because they are done elsewhere).
The level of documentation is also high, where the documentation portal and READMEs are adequate and kept up-to-date with the codebase.
Tests ran without issues, and could be easily modified to test out POCs and potential vulnerabilities.
The findings made involve the lack of sanity checks for some key config values (eg. their values should not exceed 100%). While the likelihood of the creation of governance polls to modify these values is low because the proposer loses his ANC deposit if quorum isn’t met, the impact of such polls, should they be successful, is high.
The whitelisted mappings are defined and set to true for certain token addresses, but the lines of code where they are used are commented out.
Decide whether to keep the whitelisting requirement. Either comment out the remaining lines / remove them entirely, or uncomment the lines where they are used.
It is unclear if the full opcode values include the flags as well. If so, they should be 8 bits in length, which isn’t the case. Their decimal representations are also incorrect.
| Opcode | Flags | Full Opcode | Decimal | |:-------|:------|:------------|:-------| | Deposit Stable | `FLAG_BOTH_TRANSFERS` | `0b110000` | `192` | | Redeem Stable | `FLAG_BOTH_TRANSFERS` | `0b110001` | `193` | | Repay Stable | `FLAG_INCOMING_TRANSFER` | `0b1000000` | `64` | | Lock Collateral | `FLAG_INCOMING_TRANSFER` | `0b1000001` | `65` | | Unlock Collateral | `FLAG_OUTGOING_TRANSFER` | `0b0100000` | `32` | | Borrow Stable | `FlAG_OUTGOING_TRANSFER` | `0b0100001` | `33` | | Claim Rewards | `FLAG_OUTGOING_TRANSFER` | `0b0100010` | `34` |
The opcodes were also not updated in the comments of the wormhole bridge terra contract.
Update the comments to reflect the latest changes.
| Opcode | Flags | Full Opcode | Decimal | |:-------|:------|:------------|:-------| | Deposit Stable | `FLAG_BOTH_TRANSFERS` | `0b11000000` | `192` | | Redeem Stable | `FLAG_BOTH_TRANSFERS` | `0b11000001` | `193` | | Repay Stable | `FLAG_INCOMING_TRANSFER` | `0b10000000` | `128` | | Lock Collateral | `FLAG_INCOMING_TRANSFER` | `0b10000001` | `129` | | Unlock Collateral | `FLAG_OUTGOING_TRANSFER` | `0b01000000` | `64` | | Borrow Stable | `FlAG_OUTGOING_TRANSFER` | `0b01000001` | `65` | | Claim Rewards | `FLAG_OUTGOING_TRANSFER` | `0b01000010` | `66` |
/* struct Instruction { uint8 op_code; // [1 byte] bytes32 sender_address; // [32 bytes] one_of { // [deposit_stable] opcode = 192 uint64 sequence; // [8 bytes] // [repay_stable] opcode = 128 uint64 sequence; // [8 bytes] // [unlock_collateral] opcode = 64 bytes32 collorateral_token_address; // [32 bytes] uint128 amount; // [16 bytes] // [borrow_stable] opcode = 65 uint256 amount; // [16 bytes] // [claim rewards] opcode = 66 // N/A for now // [redeem_stable] opcode = 193 uint64 sequence; // [8 bytes] // [lock_collateral] opcode = 129 uint64 sequence; // [8 bytes] } } */
https://docs.anchorprotocol.com/developers-ethereum/fees#terra-blockchain-tax
The tax amount is calculated as amount * DENOM / DENOM + tax_rate instead of amount * tax_rate / DENOM, which means the tax percentage isn’t as precise (less tax is actually charged).
We see this in the test case:
// normal tax assert_eq!( deduct_tax(&deps.as_mut().querier, Coin::new(50000000u128, "uusd")).unwrap(), Coin { denom: "uusd".to_string(), amount: Uint128::new(49504950u128) } );
Given an amount of 50_000_000, assuming a tax rate of 1%,the amount after tax would be 0.99 * 50_000_000 = 49500000, but the calculated amount is 50_000_000 * 100 / 101 ~= 49504950.
Update the calculation to be
(coin.amount.checked_sub(coin.amount.multiply_ratio( Uint128::new(1) * tax_rate, DECIMAL_FRACTION, )))?,
In the LP staking contract, it is possible to instantiate / add duplicate vesting schedules.
#[test] fn test_instantiate_duplicate_schedules() { let mut deps = mock_dependencies(&[]); let msg = InstantiateMsg { anchor_token: "reward0000".to_string(), staking_token: "staking0000".to_string(), distribution_schedule: vec![ ( mock_env().block.time.seconds() + 100, mock_env().block.time.seconds() + 200, Uint128::from(1000000u128), ), ( mock_env().block.time.seconds() + 100, mock_env().block.time.seconds() + 200, Uint128::from(1000000u128), ), ], }; let info = mock_info("addr0000", &[]); let _res = instantiate(deps.as_mut(), mock_env(), info, msg).unwrap(); }
De-duplicate the new vesting schedules when it is added. It is easier done if the schedules are sorted prior.
There is no check to ensure that max_borrow_factor is less than 100% (Decimal::One()). It is therefore possible to set a borrow factor of > 1. However, the consequence of it is negligible because it would be the same as setting the value to 100% (can’t borrow if there is no liquidity left).
Change the instantiation and update config test values to a value greater than 100%, such as Decimal256::MAX.
let msg = InstantiateMsg { owner_addr: "owner".to_string(), stable_denom: "uusd".to_string(), aterra_code_id: 123u64, anc_emission_rate: Decimal256::one(), max_borrow_factor: Decimal256::MAX, };
let msg = ExecuteMsg::UpdateConfig { owner_addr: None, interest_model: Some("interest2".to_string()), distribution_model: Some("distribution2".to_string()), max_borrow_factor: Some(Decimal256::percent(120)), };
Ensure max_borrow_factor doesn’t exceed Decimal256::one().
// add error in market/src/error.rs pub enum ContractError { #[error("Setting greater than theoretical borrow factor")] MaxTheoreticalBorrowFactorExceeded {}, ... } // in market/src/contract.rs // before store_config() in L54 if msg.max_borrow_factor > Decimal256::one() { return Err(ContractError::MaxTheoreticalBorrowFactorExceeded {}); } // in update_config() if let Some(max_borrow_factor) = max_borrow_factor { if max_borrow_factor > Decimal256::one() { return Err(ContractError::MaxTheoreticalBorrowFactorExceeded {}); } config.max_borrow_factor = max_borrow_factor; }
Safe Ratio in documentationhttps://docs.anchorprotocol.com/protocol/anchor-governance/modify-liquidation-parameters
The docs say that
“A low Safe Ratio value allows for the fast liquidation of collaterals while incurring a high price impact for the collateral, while a low Safe Ratio value enforces liquidations with lower collateral price impact, albeit with slower collateral liquidation.”
The second statement describes a high safe ratio.
Change to “while a high Safe Ratio value enforces liquidations with lower collateral price impact, albeit with slower collateral liquidation.”
Do a CMD / CTRL + F for the following statements.
form → from
// load price form the oracle → // load price from the oracle// load balance form the token contract → // load price from the token contractWITHDARW → WITHDRAW
ALICE CAN ONLY WITHDARW 40 UST → ALICE CAN ONLY WITHDRAW 40 UST// cannot liquidation collaterals → // cannot liquidate collaterals// if the token contract is already register we cannot change the address → // if the token contract is already registered we cannot change the address
is and alreadyinistantiation → instantiation
// cannot register the token at the inistantiation → // cannot register token at instantiationThere’s 1 remaining TODO that may not have been resolved.
// TODO: Should this become a reply? If so which SubMsg to make reply_on?
https://github.com/compound-finance/compound-protocol/blob/master/contracts/JumpRateModel.sol
https://docs.benqi.fi/benqi-liquidity-market/protocol-parameters
The current interest rate model used is a simple model that scales linearly with market utilization. A better interest rate model that Compound and its forks are using is the jump rate model, because it is more efficient at incentivizing liquidity at higher utilization rates.
Change the interest rate model to the jump rate model, where a kink is introduced to increase the interest multiplier after a specified market utilization rate.
#0 - GalloDaSballo
2022-08-04T23:39:33Z
L01: CrossAnchorBridge: Decide if whitelisting feature is to be kept or removed -> Dup of #44
#1 - GalloDaSballo
2022-08-04T23:44:27Z
This looks like the most complete report
parseIncomingTokenTransferInfo()Remix for prototyping + gas cost estimates
Instead of defining and incrementing the index pointer when parsing the token transfer info, it would be ~300 gas cheaper to define and utilize private constants. It also allows the encoded length check to be shifted to the front of the function, thus costing less gas should the check fail.
// start from index 0 uint256 private constant CHAIN_ID_INDEX = 0; // chainId takes first 2 bytes uint256 private constant TOKEN_RECIPIENT_INDEX = 2; // tokenRecipient takes next 32 bytes uint256 private constant TOKEN_TRANSFER_INDEX = 34; // tokenTransferSequence takes next 8 bytes uint256 private constant INSTRUCTION_SEQUENCE_INDEX = 42; // instructionSequence takes final 8 bytes uint256 private constant ENCODED_LENGTH = 50; function parseIncomingTokenTransferInfo(bytes memory encoded) public pure returns (IncomingTokenTransferInfo memory incomingTokenTransferInfo) { require( encoded.length == ENCODED_LENGTH, "invalid IncomingTokenTransferInfo encoded length" ); incomingTokenTransferInfo.chainId = encoded.toUint16(CHAIN_ID_INDEX); incomingTokenTransferInfo.tokenRecipientAddress = encoded.toBytes32( TOKEN_RECIPIENT_INDEX ); incomingTokenTransferInfo.tokenTransferSequence = encoded.toUint64( TOKEN_TRANSFER_INDEX ); incomingTokenTransferInfo.instructionSequence = encoded.toUint64( INSTRUCTION_SEQUENCE_INDEX ); }
half_dec definition in query_anc_emission_rate()It is perhaps cheaper to define half_dec as Decimal256::percent(200) instead of let half_dec = Decimal256::one() + Decimal256::one();
let half_dec = Decimal256::percent(200);
#0 - GalloDaSballo
2022-08-04T23:48:08Z
Around 300 + minor gas savings, neat that covers both Sol and Rust