Description
RAACHausePrices::setHousePrice is supposed to update the price of an asset and update the timestamp when said asset has been updated last. The current implementation though updates uint256 public lastUpdateTimestamp; globally.
Vulnerable Code
RAACHausePrices:
uint256 public lastUpdateTimestamp;
RAACHausePrices::setHousePrice:
* @notice Allows the owner to set the house price for a token
* @param _tokenId The ID of the RAAC token
* @param _amount The price to set for the house in USD
*
@> * Updates timestamp for each token individually
*/
function setHousePrice(
uint256 _tokenId,
uint256 _amount
) external onlyOracle {
tokenToHousePrice[_tokenId] = _amount;
@> lastUpdateTimestamp = block.timestamp;
emit PriceUpdated(_tokenId, _amount);
}
Looking at the code snippets we can already see quite clearly, that the intended functionality, to update the last price update timestamp individually is lacking, but the lastUpdateTimestamp for all NFTs will be updated any time any NFT will be updated.
PoC
Since the PoC is a foundry test I have added a Makefile at the end of this report to simplify installation for your convenience. Otherwise if console commands would be prefered:
First run: npm install --save-dev @nomicfoundation/hardhat-foundry
Second add: require("@nomicfoundation/hardhat-foundry"); on top of the Hardhat.Config file in the projects root directory.
Third run: npx hardhat init-foundry
And lastly, you will encounter one of the mock contracts throwing an error during compilation, this error can be circumvented by commenting out the code in entirety (ReserveLibraryMocks.sol).
And the test should be good to go:
After following above steps copy & paste the following code into ./test/invariant/PoC.t.sol and run forge test --mt test_PocTimestampOfPriceUpdatesGlobal -vv
pragma solidity ^0.8.0;
import {Test, console} from "forge-std/Test.sol";
import {StabilityPool} from "../../contracts/core/pools/StabilityPool/StabilityPool.sol";
import {LendingPool} from "../../contracts/core/pools/LendingPool/LendingPool.sol";
import {CrvUSDToken} from "../../contracts/mocks/core/tokens/crvUSDToken.sol";
import {RAACHousePrices} from "../../contracts/core/oracles/RAACHousePriceOracle.sol";
import {RAACNFT} from "../../contracts/core/tokens/RAACNFT.sol";
import {RToken} from "../../contracts/core/tokens/RToken.sol";
import {DebtToken} from "../../contracts/core/tokens/DebtToken.sol";
import {DEToken} from "../../contracts/core/tokens/DEToken.sol";
import {RAACToken} from "../../contracts/core/tokens/RAACToken.sol";
import {RAACMinter} from "../../contracts/core/minters/RAACMinter/RAACMinter.sol";
contract PoC is Test {
StabilityPool public stabilityPool;
LendingPool public lendingPool;
CrvUSDToken public crvusd;
RAACHousePrices public raacHousePrices;
RAACNFT public raacNFT;
RToken public rToken;
DebtToken public debtToken;
DEToken public deToken;
RAACToken public raacToken;
RAACMinter public raacMinter;
address owner;
address oracle;
address user1;
address user2;
address user3;
uint256 constant STARTING_TIME = 1641070800;
uint256 public currentBlockTimestamp;
uint256 constant WAD = 1e18;
uint256 constant RAY = 1e27;
function setUp() public {
vm.warp(STARTING_TIME);
currentBlockTimestamp = block.timestamp;
owner = address(this);
oracle = makeAddr("oracle");
user1 = makeAddr("user1");
user2 = makeAddr("user2");
user3 = makeAddr("user3");
uint256 initialPrimeRate = 0.1e27;
raacHousePrices = new RAACHousePrices(owner);
vm.prank(owner);
raacHousePrices.setOracle(oracle);
crvusd = new CrvUSDToken(owner);
raacNFT = new RAACNFT(address(crvusd), address(raacHousePrices), owner);
rToken = new RToken("RToken", "RToken", owner, address(crvusd));
debtToken = new DebtToken("DebtToken", "DT", owner);
deToken = new DEToken("DEToken", "DEToken", owner, address(rToken));
vm.prank(owner);
crvusd.setMinter(owner);
vm.prank(owner);
lendingPool = new LendingPool(
address(crvusd),
address(rToken),
address(debtToken),
address(raacNFT),
address(raacHousePrices),
initialPrimeRate
);
rToken.setReservePool(address(lendingPool));
debtToken.setReservePool(address(lendingPool));
rToken.transferOwnership(address(lendingPool));
debtToken.transferOwnership(address(lendingPool));
stabilityPool = new StabilityPool(address(owner));
deToken.setStabilityPool(address(stabilityPool));
raacToken = new RAACToken(owner, 0, 0);
raacMinter = new RAACMinter(address(raacToken), address(stabilityPool), address(lendingPool), owner);
stabilityPool.initialize(address(rToken), address(deToken), address(raacToken), address(raacMinter), address(crvusd), address(lendingPool));
vm.prank(owner);
raacToken.setMinter(address(raacMinter));
crvusd.mint(address(attacker), type(uint128).max);
crvusd.mint(user1, type(uint128).max);
crvusd.mint(user2, type(uint128).max);
crvusd.mint(user3, type(uint128).max);
}
function test_PocTimestampOfPriceUpdatesGlobal() public {
vm.warp(14231234);
vm.startPrank(oracle);
raacHousePrices.setHousePrice(1, 10e18);
vm.stopPrank();
vm.startPrank(user1);
crvusd.approve(address(raacNFT), 10e18);
raacNFT.mint(1, 10e18);
vm.stopPrank();
(, uint256 lastUpdateId1) = raacHousePrices.getLatestPrice(1);
console.log(lastUpdateId1);
assertEq(lastUpdateId1, 14231234);
vm.warp(14231900);
vm.startPrank(oracle);
raacHousePrices.setHousePrice(2, 10e18);
vm.stopPrank();
(, lastUpdateId1) = raacHousePrices.getLatestPrice(1);
console.log(lastUpdateId1);
assertEq(14231900, lastUpdateId1);
}
}
Running that test produces the following log:
Ran 1 test for test/invariant/PoC.t.sol:PoC
[PASS] test_PocTimestampOfPriceUpdatesGlobal() (gas: 279897)
Logs:
14231234
14231900
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 17.09ms (1.68ms CPU time)
Showcasing indeed, that the price update of NFT ID 2 changed the last update timestamp of ID1.
Impact
In the current implementation it is basically impossible to check when exactly a specific tokenId has been updated, because as soon as any ID will be updated, all prices will seem to be updated as well, even it is not true.
The implications are relatively obvious, since there can not be any validation on chain if the price is stale for too long (and even off-chain would be difficult since the state simply will be read wrong by off-chain tooling), it is easily possible that many prices are stale, which results in users either missing out on potential higher credit lines or, which is worse, users would potentially be due for liquidation / can borrow more than they should harming the solvency of the protocol.
Looking at the natspec, it also seems this functionality was simply forgotten.
With a High Likelihood, since the issue occurs at any price update and a High impact, due to the fact that the protocol might be carrying unknowingly bad debt, I rate the total severity as High.
Used Tools
Manual Review
Recommended Mitigation
A possible mitigation would include simply connecting a specific asset ID to the timestamp like shown here:
contract RAACHousePrices is Ownable {
- mapping(uint256 => uint256) public tokenToHousePrice;
- uint256 public lastUpdateTimestamp;
+ struct PriceData {
+ uint256 price;
+ uint256 lastUpdateTimestamp;
+ }
+ mapping(uint256 => PriceData) public tokenToPriceData;
address public oracle;
/// *snip* ///
function setHousePrice(
uint256 _tokenId,
uint256 _amount
) external onlyOracle {
- tokenToHousePrice[_tokenId] = _amount;
- lastUpdateTimestamp = block.timestamp;
+ tokenToPriceData[_tokenId] = PriceData({
+ price: _amount,
+ lastUpdateTimestamp: block.timestamp
+ });
emit PriceUpdated(_tokenId, _amount);
}
/// *snip* ///
}
Appendix
Copy the following import into your Hardhat.Config file in the projects root dir:
require("@nomicfoundation/hardhat-foundry");
Paste the following into a new file "Makefile" into the projects root directory:
.PHONY: install-foundry init-foundry all
install-foundry:
npm install --save-dev @nomicfoundation/hardhat-foundry
init-foundry: install-foundry
npx hardhat init-foundry
# Default target that runs everything in sequence
all: install-foundry init-foundry
And run make all
